[PATCH v5 00/10] Add the I3C subsystem

[PATCH v5 00/10] Add the I3C subsystem

[Boris Brezillon]

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

Only minor things have changes since v3, 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                                 |  620 +++++++
 drivers/i3c/device.c                               |  294 ++++
 drivers/i3c/internals.h                            |   28 +
 drivers/i3c/master.c                               | 1722 ++++++++++++++++++++
 drivers/i3c/master/Kconfig                         |    5 +
 drivers/i3c/master/Makefile                        |    1 +
 drivers/i3c/master/i3c-master-cdns.c               | 1651 +++++++++++++++++++
 include/dt-bindings/i3c/i3c.h                      |   28 +
 include/linux/i3c/ccc.h                            |  382 +++++
 include/linux/i3c/device.h                         |  307 ++++
 include/linux/i3c/master.h                         |  587 +++++++
 include/linux/mod_devicetable.h                    |   17 +
 29 files changed, 6658 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

-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 02/10] docs: driver-api: Add I3C documentation

[Boris Brezillon]

Add the I3C documentation describing the protocol, the master driver API
and the device driver API.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v5:
- Remove useless conf.py file
- Add SPDX headers

Changes in v2:
- Moved out of patch "i3c: Add core I3C infrastructure"
- Add link to the I3C spec
- Move rst files in Documentation/driver-api/i3c/
---
 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 +
 5 files changed, 234 insertions(+)
 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

diff --git a/Documentation/driver-api/i3c/device-driver-api.rst b/Documentation/driver-api/i3c/device-driver-api.rst
new file mode 100644
index 000000000000..85bc3381cd3e
--- /dev/null
+++ b/Documentation/driver-api/i3c/device-driver-api.rst
@@ -0,0 +1,9 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=====================
+I3C device driver API
+=====================
+
+.. kernel-doc:: include/linux/i3c/device.h
+
+.. kernel-doc:: drivers/i3c/device.c
diff --git a/Documentation/driver-api/i3c/index.rst b/Documentation/driver-api/i3c/index.rst
new file mode 100644
index 000000000000..783d6dad054b
--- /dev/null
+++ b/Documentation/driver-api/i3c/index.rst
@@ -0,0 +1,11 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=============
+I3C subsystem
+=============
+
+.. toctree::
+
+   protocol
+   device-driver-api
+   master-driver-api
diff --git a/Documentation/driver-api/i3c/master-driver-api.rst b/Documentation/driver-api/i3c/master-driver-api.rst
new file mode 100644
index 000000000000..bb19264aa239
--- /dev/null
+++ b/Documentation/driver-api/i3c/master-driver-api.rst
@@ -0,0 +1,10 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+================================
+I3C master controller driver API
+================================
+
+.. kernel-doc:: drivers/i3c/master.c
+
+.. kernel-doc:: include/linux/i3c/master.h
+
diff --git a/Documentation/driver-api/i3c/protocol.rst b/Documentation/driver-api/i3c/protocol.rst
new file mode 100644
index 000000000000..a768afa7f12a
--- /dev/null
+++ b/Documentation/driver-api/i3c/protocol.rst
@@ -0,0 +1,203 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+============
+I3C protocol
+============
+
+Disclaimer
+==========
+
+This chapter will focus on aspects that matter to software developers. For
+everything hardware related (like how things are transmitted on the bus, how
+collisions are prevented, ...) please have a look at the I3C specification.
+
+This document is just a brief introduction to the I3C protocol and the concepts
+it brings on the table. If you need more information, please refer to the MIPI
+I3C specification (can be downloaded here
+http://resources.mipi.org/mipi-i3c-v1-download).
+
+Introduction
+============
+
+The I3C (pronounced 'eye-three-see') is a MIPI standardized protocol designed
+to overcome I2C limitations (limited speed, external signals needed for
+interrupts, no automatic detection of the devices connected to the bus, ...)
+while remaining power-efficient.
+
+I3C Bus
+=======
+
+An I3C bus is made of several I3C devices and possibly some I2C devices as
+well, but let's focus on I3C devices for now.
+
+An I3C device on the I3C bus can have one of the following roles:
+
+* Master: the device is driving the bus. It's the one in charge of initiating
+  transactions or deciding who is allowed to talk on the bus (slave generated
+  events are possible in I3C, see below).
+* Slave: the device acts as a slave, and is not able to send frames to another
+  slave on the bus. The device can still send events to the master on
+  its own initiative if the master allowed it.
+
+I3C is a multi-master protocol, so there might be several masters on a bus,
+though only one device can act as a master at a given time. In order to gain
+bus ownership, a master has to follow a specific procedure.
+
+Each device on the I3C bus has to be assigned a dynamic address to be able to
+communicate. Until this is done, the device should only respond to a limited
+set of commands. If it has a static address (also called legacy I2C address),
+the device can reply to I2C transfers.
+
+In addition to these per-device addresses, the protocol defines a broadcast
+address in order to address all devices on the bus.
+
+Once a dynamic address has been assigned to a device, this address will be used
+for any direct communication with the device. Note that even after being
+assigned a dynamic address, the device should still process broadcast messages.
+
+I3C Device discovery
+====================
+
+The I3C protocol defines a mechanism to automatically discover devices present
+on the bus, their capabilities and the functionalities they provide. In this
+regard I3C is closer to a discoverable bus like USB than it is to I2C or SPI.
+
+The discovery mechanism is called DAA (Dynamic Address Assignment), because it
+not only discovers devices but also assigns them a dynamic address.
+
+During DAA, each I3C device reports 3 important things:
+
+* BCR: Bus Characteristic Register. This 8-bit register describes the device bus
+  related capabilities
+* DCR: Device Characteristic Register. This 8-bit register describes the
+  functionalities provided by the device
+* Provisional ID: A 48-bit unique identifier. On a given bus there should be no
+  Provisional ID collision, otherwise the discovery mechanism may fail.
+
+I3C slave events
+================
+
+The I3C protocol allows slaves to generate events on their own, and thus allows
+them to take temporary control of the bus.
+
+This mechanism is called IBI for In Band Interrupts, and as stated in the name,
+it allows devices to generate interrupts without requiring an external signal.
+
+During DAA, each device on the bus has been assigned an address, and this
+address will serve as a priority identifier to determine who wins if 2 different
+devices are generating an interrupt at the same moment on the bus (the lower the
+dynamic address the higher the priority).
+
+Masters are allowed to inhibit interrupts if they want to. This inhibition
+request can be broadcasted (applies to all devices) or sent to a specific
+device.
+
+I3C Hot-Join
+============
+
+The Hot-Join mechanism is similart to USB hotplug. This mechanism allows
+slaves to join the bus after it has been initialized by the master.
+
+This covers the following use cases:
+
+* the device is not powered when the bus is probed
+* the device is hotplugged on the bus through an extension board
+
+This mechanism is relying on slave events to inform the master that a new
+device joined the bus and is waiting for a dynamic address.
+
+The master is then free to address the request as it wishes: ignore it or
+assign a dynamic address to the slave.
+
+I3C transfer types
+==================
+
+If you omit SMBus (which is just a standardization on how to access registers
+exposed by I2C devices), I2C has only one transfer type.
+
+I3C defines 3 different classes of transfer in addition to I2C transfers which
+are here for backward compatibility with I2C devices.
+
+I3C CCC commands
+----------------
+
+CCC (Common Command Code) commands are meant to be used for anything that is
+related to bus management and all features that are common to a set of devices.
+
+CCC commands contain an 8-bit CCC id describing the command that is executed.
+The MSB of this id specifies whether this is a broadcast command (bit7 = 0) or a
+unicast one (bit7 = 1).
+
+The command ID can be followed by a payload. Depending on the command, this
+payload is either sent by the master sending the command (write CCC command),
+or sent by the slave receiving the command (read CCC command). Of course, read
+accesses only apply to unicast commands.
+Note that, when sending a CCC command to a specific device, the device address
+is passed in the first byte of the payload.
+
+The payload length is not explicitly passed on the bus, and should be extracted
+from the CCC id.
+
+Note that vendors can use a dedicated range of CCC ids for their own commands
+(0x61-0x7f and 0xe0-0xef).
+
+I3C Private SDR transfers
+-------------------------
+
+Private SDR (Single Data Rate) transfers should be used for anything that is
+device specific and does not require high transfer speed.
+
+It is the equivalent of I2C transfers but in the I3C world. Each transfer is
+passed the device address (dynamic address assigned during DAA), a payload
+and a direction.
+
+The only difference with I2C is that the transfer is much faster (typical SCL
+frequency is 12.5MHz).
+
+I3C HDR commands
+----------------
+
+HDR commands should be used for anything that is device specific and requires
+high transfer speed.
+
+The first thing attached to an HDR command is the HDR mode. There are currently
+3 different modes defined by the I3C specification (refer to the specification
+for more details):
+
+* HDR-DDR: Double Data Rate mode
+* HDR-TSP: Ternary Symbol Pure. Only usable on busses with no I2C devices
+* HDR-TSL: Ternary Symbol Legacy. Usable on busses with I2C devices
+
+When sending an HDR command, the whole bus has to enter HDR mode, which is done
+using a broadcast CCC command.
+Once the bus has entered a specific HDR mode, the master sends the HDR command.
+An HDR command is made of:
+
+* one 16-bits command word
+* N 16-bits data words
+
+Those words may be wrapped with specific preambles/post-ambles which depend on
+the chosen HDR mode and are detailed here (see the specification for more
+details).
+
+The 16-bits command word is made of:
+
+* bit[15]: direction bit, read is 1 write is 0
+* bit[14:8]: command code. Identifies the command being executed, the amount of
+  data words and their meaning
+* bit[7:1]: I3C address of the device this command is addressed to
+* bit[0]: reserved/parity-bit
+
+Backward compatibility with I2C devices
+=======================================
+
+The I3C protocol has been designed to be backward compatible with I2C devices.
+This backward compatibility allows one to connect a mix of I2C and I3C devices
+on the same bus, though, in order to be really efficient, I2C devices should
+be equipped with 50 ns spike filters.
+
+I2C devices can't be discovered like I3C ones and have to be statically
+declared. In order to let the master know what these devices are capable of
+(both in terms of bus related limitations and functionalities), the software
+has to provide some information, which is done through the LVR (Legacy I2C
+Virtual Register).
diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst
index 6d9f2f9fe20e..cc6a33f232ea 100644
--- a/Documentation/driver-api/index.rst
+++ b/Documentation/driver-api/index.rst
@@ -32,6 +32,7 @@ available subsections can be seen below.
    pci
    spi
    i2c
+   i3c/index
    hsi
    edac
    scsi
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 03/10] i3c: Add sysfs ABI spec

[Boris Brezillon]

Document sysfs files/directories/symlinks exposed by the I3C subsystem.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v5:
- Fix the kernel version
- Rename address into dynamic_address to match changes done in the code

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- new patch
---
 Documentation/ABI/testing/sysfs-bus-i3c | 95 +++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i3c

diff --git a/Documentation/ABI/testing/sysfs-bus-i3c b/Documentation/ABI/testing/sysfs-bus-i3c
new file mode 100644
index 000000000000..068fd1e97c01
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-i3c
@@ -0,0 +1,95 @@
+What:		/sys/bus/i3c/devices/i3c-<bus-id>
+KernelVersion:  4.19
+Contact:	linux-i3c@vger.kernel.org
+Description:
+		An I3C bus. This directory will contain one sub-directory per
+		I3C device present on the bus.
+
+What:		/sys/bus/i3c/devices/i3c-<bus-id>/current_master
+KernelVersion:  4.19
+Contact:	linux-i3c@vger.kernel.org
+Description:
+		Expose the master that owns the bus (<bus-id>-<master-pid>) at
+		the time this file is read. Note that bus ownership can change
+		overtime, so there's no guarantee that when the read() call
+		returns, the value returned is still valid.
+
+What:		/sys/bus/i3c/devices/i3c-<bus-id>/mode
+KernelVersion:  4.19
+Contact:	linux-i3c@vger.kernel.org
+Description:
+		I3C bus mode. Can be "pure", "mixed-fast" or "mixed-slow". See
+		the I3C specification for a detailed description of what each
+		of these modes implies.
+
+What:		/sys/bus/i3c/devices/i3c-<bus-id>/i3c_scl_frequency
+KernelVersion:  4.19
+Contact:	linux-i3c@vger.kernel.org
+Description:
+		The frequency (expressed in Hz) of the SCL signal when
+		operating in I3C SDR mode.
+
+What:		/sys/bus/i3c/devices/i3c-<bus-id>/i2c_scl_frequency
+KernelVersion:  4.19
+Contact:	linux-i3c@vger.kernel.org
+Description:
+		The frequency (expressed in Hz) of the SCL signal when
+		operating in I2C mode.
+
+What:		/sys/bus/i3c/devices/i3c-<bus-id>/<bus-id>-<device-pid>
+KernelVersion:  4.19
+Contact:	linux-i3c@vger.kernel.org
+Description:
+		An I3C device present on I3C bus identified by <bus-id>. Note
+		that all devices are represented including the master driving
+		the bus.
+
+What:		/sys/bus/i3c/devices/i3c-<bus-id>/<bus-id>-<device-pid>/dynamic_address
+KernelVersion:  4.19
+Contact:	linux-i3c@vger.kernel.org
+Description:
+		Dynamic address assigned to device <bus-id>-<device-pid>. This
+		address may change if the bus is re-initialized.
+
+What:		/sys/bus/i3c/devices/i3c-<bus-id>/<bus-id>-<device-pid>/bcr
+KernelVersion:  4.19
+Contact:	linux-i3c@vger.kernel.org
+Description:
+		BCR stands for Bus Characteristics Register and express the
+		device capabilities in term of speed, maximum read/write
+		length, etc. See the I3C specification for more details.
+
+What:		/sys/bus/i3c/devices/i3c-<bus-id>/<bus-id>-<device-pid>/dcr
+KernelVersion:  4.19
+Contact:	linux-i3c@vger.kernel.org
+Description:
+		DCR stands for Device Characteristics Register and express the
+		device capabilities in term of exposed features. See the I3C
+		specification for more details.
+
+What:		/sys/bus/i3c/devices/i3c-<bus-id>/<bus-id>-<device-pid>/pid
+KernelVersion:  4.19
+Contact:	linux-i3c@vger.kernel.org
+Description:
+		PID stands for Provisional ID and is used to uniquely identify
+		a device on a bus. This PID contains information about the
+		vendor, the part and an instance ID so that several devices of
+		the same type can be connected on the same bus.
+		See the I3C specification for more details.
+
+What:		/sys/bus/i3c/devices/i3c-<bus-id>/<bus-id>-<device-pid>/hdrcap
+KernelVersion:  4.19
+Contact:	linux-i3c@vger.kernel.org
+Description:
+		Expose the HDR (High Data Rate) capabilities of a device.
+		Returns a list of supported HDR mode, each element is separated
+		by space. Modes can be "hdr-ddr", "hdr-tsp" and "hdr-tsl".
+		See the I3C specification for more details about these HDR
+		modes.
+
+What:		/sys/bus/i3c/devices/<bus-id>-<device-pid>
+KernelVersion:  4.19
+Contact:	linux-i3c@vger.kernel.org
+Description:
+		These directories are just symbolic links to
+		/sys/bus/i3c/devices/i3c-<bus-id>/<bus-id>-<device-pid>.
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 04/10] dt-bindings: i3c: Document core bindings

[Boris Brezillon]

A new I3C subsystem has been added and a generic description has been
created to represent the I3C bus and the devices connected on it.

Document this generic representation.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v5:
- Add Rob's R-b

Changes in v4:
- Clarify the fact that static address == I3C address and dynamic
  address == I3C address
- Use i2c-scl-hz in the example

Changes in v3:
- Rename {i2c,i3c}-scl-frequency DT prop into {i2c,i3c}-scl-hz
- Rework the way we expose the provisional ID and LVR information
- Rename dynamic-address into assigned-address
- Enforce the I3C master node name

Changes in v2:
- Define how to describe I3C devices in the DT and when it should be
  used. Note that the parsing of I3C devices is not yet implemented in
  the framework. Will be added when someone really needs it.
---
 Documentation/devicetree/bindings/i3c/i3c.txt | 140 ++++++++++++++++++++++++++
 1 file changed, 140 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i3c/i3c.txt

diff --git a/Documentation/devicetree/bindings/i3c/i3c.txt b/Documentation/devicetree/bindings/i3c/i3c.txt
new file mode 100644
index 000000000000..13b719f1ef15
--- /dev/null
+++ b/Documentation/devicetree/bindings/i3c/i3c.txt
@@ -0,0 +1,140 @@
+Generic device tree bindings for I3C busses
+===========================================
+
+This document describes generic bindings that should be used to describe I3C
+busses in a device tree.
+
+Required properties
+-------------------
+
+- #address-cells  - should be <3>. Read more about addresses below.
+- #size-cells     - should be <0>.
+- compatible      - name of the I3C master controller driving the I3C bus
+
+For other required properties e.g. to describe register sets,
+clocks, etc. check the binding documentation of the specific driver.
+The node describing an I3C bus should be named i3c-master.
+
+Optional properties
+-------------------
+
+These properties may not be supported by all I3C master drivers. Each I3C
+master bindings should specify which of them are supported.
+
+- i3c-scl-hz: frequency of the SCL signal used for I3C transfers.
+	      When undefined the core sets it to 12.5MHz.
+
+- i2c-scl-hz: frequency of the SCL signal used for I2C transfers.
+	      When undefined, the core looks at LVR (Legacy Virtual Register)
+	      values of I2C devices described in the device tree to determine
+	      the maximum I2C frequency.
+
+I2C devices
+===========
+
+Each I2C device connected to the bus should be described in a subnode. All
+properties described in Documentation/devicetree/bindings/i2c/i2c.txt are
+valid here, but several new properties have been added.
+
+New constraint on existing properties:
+--------------------------------------
+- reg: contains 3 cells
+  + first cell : still encoding the I2C address
+
+  + second cell: should have bit 31 set to 1 to signify that this is an I2C
+		 device. Bits 0 to 7 encode the I3C LVR (Legacy Virtual
+		 Register):
+
+	bit[7:5]: I2C device index. Possible values
+	* 0: I2C device has a 50 ns spike filter
+	* 1: I2C device does not have a 50 ns spike filter but supports high
+	     frequency on SCL
+	* 2: I2C device does not have a 50 ns spike filter and is not tolerant
+	     to high frequencies
+	* 3-7: reserved
+
+	bit[4]: tell whether the device operates in FM (Fast Mode) or FM+ mode
+	* 0: FM+ mode
+	* 1: FM mode
+
+	bit[3:0]: device type
+	* 0-15: reserved
+
+  + third cell: should be 0
+
+I3C devices
+===========
+
+All I3C devices are supposed to support DAA (Dynamic Address Assignment), and
+are thus discoverable. So, by default, I3C devices do not have to be described
+in the device tree.
+This being said, one might want to attach extra resources to these devices,
+and those resources may have to be described in the device tree, which in turn
+means we have to describe I3C devices.
+
+Another use case for describing an I3C device in the device tree is when this
+I3C device has a static I2C address and we want to assign it a specific I3C
+dynamic address before the DAA takes place (so that other devices on the bus
+can't take this dynamic address).
+
+The I3C device should be names <device-type>@<static-i2c-address>,<i3c-pid>,
+where device-type is describing the type of device connected on the bus
+(gpio-controller, sensor, ...).
+
+Required properties
+-------------------
+- reg: contains 3 cells
+  + first cell : encodes the static I2C address. Should be 0 if the device does
+		 not have one (0 is not a valid I2C address).
+
+  + second and third cells: should encode the ProvisionalID. The second cell
+			    contains the manufacturer ID left-shifted by 1.
+			    The third cell contains ORing of the part ID
+			    left-shifted by 16, the instance ID left-shifted
+			    by 12 and the extra information. This encoding is
+			    following the PID definition provided by the I3C
+			    specification.
+
+Optional properties
+-------------------
+- assigned-address: dynamic address to be assigned to this device. This
+		    property is only valid if the I3C device has a static
+		    address (first cell of the reg property != 0).
+
+
+Example:
+
+	i3c-master@d040000 {
+		compatible = "cdns,i3c-master";
+		clocks = <&coreclock>, <&i3csysclock>;
+		clock-names = "pclk", "sysclk";
+		interrupts = <3 0>;
+		reg = <0x0d040000 0x1000>;
+		#address-cells = <3>;
+		#size-cells = <0>;
+
+		status = "okay";
+		i2c-scl-hz = <100000>;
+
+		/* I2C device. */
+		nunchuk: nunchuk@52 {
+			compatible = "nintendo,nunchuk";
+			reg = <0x52 0x80000010 0x0>;
+		};
+
+		/* I3C device with a static I2C address. */
+		thermal_sensor: sensor@68,39200144004 {
+			reg = <0x68 0x392 0x144004>;
+			assigned-address = <0xa>;
+		};
+
+		/*
+		 * I3C device without a static I2C address but requiring
+		 * resources described in the DT.
+		 */
+		sensor@0,39200154004 {
+			reg = <0x0 0x392 0x154004>;
+			clocks = <&clock_provider 0>;
+		};
+	};
+
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 05/10] dt-bindings: i3c: Add macros to help fill I3C/I2C device's reg property

[Boris Brezillon]

The reg property of devices connected to an I3C bus have 3 cells, and
filling them manually is not trivial. Provides macros to help doing
that.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v5:
- none
---
 include/dt-bindings/i3c/i3c.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 include/dt-bindings/i3c/i3c.h

diff --git a/include/dt-bindings/i3c/i3c.h b/include/dt-bindings/i3c/i3c.h
new file mode 100644
index 000000000000..97448c546649
--- /dev/null
+++ b/include/dt-bindings/i3c/i3c.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2017 Cadence Design Systems Inc.
+ *
+ * Author: Boris Brezillon <boris.brezillon@bootlin.com>
+ */
+
+#ifndef _DT_BINDINGS_I3C_I3C_H
+#define _DT_BINDINGS_I3C_I3C_H
+
+#define IS_I2C_DEV		0x80000000
+
+#define I2C_DEV(addr, lvr)					\
+	(addr) (IS_I2C_DEV | (lvr)) 0x0
+
+#define I3C_PID(manufid, partid, instid, extrainfo)		\
+	((manufid) << 1)					\
+	(((partid) << 16) | ((instid) << 12) | (extrainfo))
+
+#define I3C_DEV_WITH_STATIC_ADDR(addr, manufid, partid,		\
+				 instid, extrainfo)		\
+	(addr) I3C_PID(manufid, partid, instid, extrainfo)
+
+#define I3C_DEV(manufid, partid, instid, extrainfo)		\
+	I3C_DEV_WITH_STATIC_ADDR(0x0, manufid, partid,		\
+				 instid, extrainfo)
+
+#endif
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 06/10] MAINTAINERS: Add myself as the I3C subsystem maintainer

[Boris Brezillon]

Create an entry for the I3C subsystem and mark it as maintained by me.
There's no official git repository, patchwork instance, mailing list or
website yet, but this will be added after the subsystem has been
accepted.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v5:
- Add the sysfs ABI file
---
 MAINTAINERS | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9d5eeff51b5f..2ab9a64743b2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6764,6 +6764,16 @@ L:	linux-i2c@vger.kernel.org
 S:	Maintained
 F:	drivers/i2c/i2c-stub.c
 
+I3C SUBSYSTEM
+M:	Boris Brezillon <boris.brezillon@bootlin.com>
+S:	Maintained
+F:	Documentation/ABI/testing/sysfs-bus-i3c
+F:	Documentation/devicetree/bindings/i3c/
+F:	Documentation/driver-api/i3c
+F:	drivers/i3c/
+F:	include/linux/i3c/
+F:	include/dt-bindings/i3c/
+
 IA64 (Itanium) PLATFORM
 M:	Tony Luck <tony.luck@intel.com>
 M:	Fenghua Yu <fenghua.yu@intel.com>
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 07/10] i3c: master: Add driver for Cadence IP

[Boris Brezillon]

Add a driver for Cadence I3C master IP.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v5:
- Drop unused len var in cdns_i3c_master_handle_ibi()
- Get IBIR and CMDR depth from CONF_STATUS0
- s/2017/2018/ in copyright header
- Fix coding style issues

Changes in v4:
- Fix potential unsigned integer underflow
- Add missing static specific on IBI related functions

Changes in v3:
- Adjust to match I3C framework changes
- Implement support the CMD RESPONSE QUEUE and IBI QUEUE added in the
  latest revision of Cadence master IP
- Remove support for HDR modes

Changes in v2:
- Add basic IBI support. Note that the IP is not really reliable with
  regards to IBI because you can't extract IBI payloads as soon as you
  have more than one IBI waiting in the HW queue. This is something
  that will hopefully be addressed in future revisions of this IP
- Add a simple xfer queueing mechanism to optimize message queuing.
- Fix a few bugs
- Add support for Hot Join
---
 drivers/i3c/master/Kconfig           |    5 +
 drivers/i3c/master/Makefile          |    1 +
 drivers/i3c/master/i3c-master-cdns.c | 1651 ++++++++++++++++++++++++++++++++++
 3 files changed, 1657 insertions(+)
 create mode 100644 drivers/i3c/master/i3c-master-cdns.c

diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig
index e69de29bb2d1..56b9a18543b2 100644
--- a/drivers/i3c/master/Kconfig
+++ b/drivers/i3c/master/Kconfig
@@ -0,0 +1,5 @@
+config CDNS_I3C_MASTER
+	tristate "Cadence I3C master driver"
+	depends on I3C
+	help
+	  Enable this driver if you want to support Cadence I3C master block.
diff --git a/drivers/i3c/master/Makefile b/drivers/i3c/master/Makefile
index e69de29bb2d1..4c4304aa9534 100644
--- a/drivers/i3c/master/Makefile
+++ b/drivers/i3c/master/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_CDNS_I3C_MASTER)		+= i3c-master-cdns.o
diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
new file mode 100644
index 000000000000..4ca804f93f42
--- /dev/null
+++ b/drivers/i3c/master/i3c-master-cdns.c
@@ -0,0 +1,1651 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Cadence Design Systems Inc.
+ *
+ * Author: Boris Brezillon <boris.brezillon@bootlin.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/i3c/master.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/workqueue.h>
+
+#define DEV_ID				0x0
+#define DEV_ID_I3C_MASTER		0x5034
+
+#define CONF_STATUS0			0x4
+#define CONF_STATUS0_CMDR_DEPTH(x)	(4 << (((x) & GENMASK(31, 29)) >> 29))
+#define CONF_STATUS0_ECC_CHK		BIT(28)
+#define CONF_STATUS0_INTEG_CHK		BIT(27)
+#define CONF_STATUS0_CSR_DAP_CHK	BIT(26)
+#define CONF_STATUS0_TRANS_TOUT_CHK	BIT(25)
+#define CONF_STATUS0_PROT_FAULTS_CHK	BIT(24)
+#define CONF_STATUS0_GPO_NUM(x)		(((x) & GENMASK(23, 16)) >> 16)
+#define CONF_STATUS0_GPI_NUM(x)		(((x) & GENMASK(15, 8)) >> 8)
+#define CONF_STATUS0_IBIR_DEPTH(x)	(4 << (((x) & GENMASK(7, 6)) >> 7))
+#define CONF_STATUS0_SUPPORTS_DDR	BIT(5)
+#define CONF_STATUS0_SEC_MASTER		BIT(4)
+#define CONF_STATUS0_DEVS_NUM(x)	((x) & GENMASK(3, 0))
+
+#define CONF_STATUS1			0x8
+#define CONF_STATUS1_IBI_HW_RES(x)	((((x) & GENMASK(31, 28)) >> 28) + 1)
+#define CONF_STATUS1_CMD_DEPTH(x)	(4 << (((x) & GENMASK(27, 26)) >> 26))
+#define CONF_STATUS1_SLVDDR_RX_DEPTH(x)	(8 << (((x) & GENMASK(25, 21)) >> 21))
+#define CONF_STATUS1_SLVDDR_TX_DEPTH(x)	(8 << (((x) & GENMASK(20, 16)) >> 16))
+#define CONF_STATUS1_IBI_DEPTH(x)	(2 << (((x) & GENMASK(12, 10)) >> 10))
+#define CONF_STATUS1_RX_DEPTH(x)	(8 << (((x) & GENMASK(9, 5)) >> 5))
+#define CONF_STATUS1_TX_DEPTH(x)	(8 << ((x) & GENMASK(4, 0)))
+
+#define REV_ID				0xc
+#define REV_ID_VID(id)			(((id) & GENMASK(31, 20)) >> 20)
+#define REV_ID_PID(id)			(((id) & GENMASK(19, 8)) >> 8)
+#define REV_ID_REV_MAJOR(id)		(((id) & GENMASK(7, 4)) >> 4)
+#define REV_ID_REV_MINOR(id)		((id) & GENMASK(3, 0))
+
+#define CTRL				0x10
+#define CTRL_DEV_EN			BIT(31)
+#define CTRL_HALT_EN			BIT(30)
+#define CTRL_MCS			BIT(29)
+#define CTRL_MCS_EN			BIT(28)
+#define CTRL_HJ_DISEC			BIT(8)
+#define CTRL_MST_ACK			BIT(7)
+#define CTRL_HJ_ACK			BIT(6)
+#define CTRL_HJ_INIT			BIT(5)
+#define CTRL_MST_INIT			BIT(4)
+#define CTRL_AHDR_OPT			BIT(3)
+#define CTRL_PURE_BUS_MODE		0
+#define CTRL_MIXED_FAST_BUS_MODE	2
+#define CTRL_MIXED_SLOW_BUS_MODE	3
+#define CTRL_BUS_MODE_MASK		GENMASK(1, 0)
+
+#define PRESCL_CTRL0			0x14
+#define PRESCL_CTRL0_I2C(x)		((x) << 16)
+#define PRESCL_CTRL0_I3C(x)		(x)
+#define PRESCL_CTRL0_MAX		GENMASK(9, 0)
+
+#define PRESCL_CTRL1			0x18
+#define PRESCL_CTRL1_PP_LOW_MASK	GENMASK(15, 8)
+#define PRESCL_CTRL1_PP_LOW(x)		((x) << 8)
+#define PRESCL_CTRL1_OD_LOW_MASK	GENMASK(7, 0)
+#define PRESCL_CTRL1_OD_LOW(x)		(x)
+
+#define MST_IER				0x20
+#define MST_IDR				0x24
+#define MST_IMR				0x28
+#define MST_ICR				0x2c
+#define MST_ISR				0x30
+#define MST_INT_HALTED			BIT(18)
+#define MST_INT_MR_DONE			BIT(17)
+#define MST_INT_IMM_COMP		BIT(16)
+#define MST_INT_TX_THR			BIT(15)
+#define MST_INT_TX_OVF			BIT(14)
+#define MST_INT_IBID_THR		BIT(12)
+#define MST_INT_IBID_UNF		BIT(11)
+#define MST_INT_IBIR_THR		BIT(10)
+#define MST_INT_IBIR_UNF		BIT(9)
+#define MST_INT_IBIR_OVF		BIT(8)
+#define MST_INT_RX_THR			BIT(7)
+#define MST_INT_RX_UNF			BIT(6)
+#define MST_INT_CMDD_EMP		BIT(5)
+#define MST_INT_CMDD_THR		BIT(4)
+#define MST_INT_CMDD_OVF		BIT(3)
+#define MST_INT_CMDR_THR		BIT(2)
+#define MST_INT_CMDR_UNF		BIT(1)
+#define MST_INT_CMDR_OVF		BIT(0)
+
+#define MST_STATUS0			0x34
+#define MST_STATUS0_IDLE		BIT(18)
+#define MST_STATUS0_HALTED		BIT(17)
+#define MST_STATUS0_MASTER_MODE		BIT(16)
+#define MST_STATUS0_TX_FULL		BIT(13)
+#define MST_STATUS0_IBID_FULL		BIT(12)
+#define MST_STATUS0_IBIR_FULL		BIT(11)
+#define MST_STATUS0_RX_FULL		BIT(10)
+#define MST_STATUS0_CMDD_FULL		BIT(9)
+#define MST_STATUS0_CMDR_FULL		BIT(8)
+#define MST_STATUS0_TX_EMP		BIT(5)
+#define MST_STATUS0_IBID_EMP		BIT(4)
+#define MST_STATUS0_IBIR_EMP		BIT(3)
+#define MST_STATUS0_RX_EMP		BIT(2)
+#define MST_STATUS0_CMDD_EMP		BIT(1)
+#define MST_STATUS0_CMDR_EMP		BIT(0)
+
+#define CMDR				0x38
+#define CMDR_NO_ERROR			0
+#define CMDR_DDR_PREAMBLE_ERROR		1
+#define CMDR_DDR_PARITY_ERROR		2
+#define CMDR_DDR_RX_FIFO_OVF		3
+#define CMDR_DDR_TX_FIFO_UNF		4
+#define CMDR_M0_ERROR			5
+#define CMDR_M1_ERROR			6
+#define CMDR_M2_ERROR			7
+#define CMDR_MST_ABORT			8
+#define CMDR_NACK_RESP			9
+#define CMDR_INVALID_DA			10
+#define CMDR_DDR_DROPPED		11
+#define CMDR_ERROR(x)			(((x) & GENMASK(27, 24)) >> 24)
+#define CMDR_XFER_BYTES(x)		(((x) & GENMASK(19, 8)) >> 8)
+#define CMDR_CMDID_HJACK_DISEC		0xfe
+#define CMDR_CMDID_HJACK_ENTDAA		0xff
+#define CMDR_CMDID(x)			((x) & GENMASK(7, 0))
+
+#define IBIR				0x3c
+#define IBIR_ACKED			BIT(12)
+#define IBIR_SLVID(x)			(((x) & GENMASK(11, 8)) >> 8)
+#define IBIR_ERROR			BIT(7)
+#define IBIR_XFER_BYTES(x)		(((x) & GENMASK(6, 2)) >> 2)
+#define IBIR_TYPE_IBI			0
+#define IBIR_TYPE_HJ			1
+#define IBIR_TYPE_MR			2
+#define IBIR_TYPE(x)			((x) & GENMASK(1, 0))
+
+#define SLV_IER				0x40
+#define SLV_IDR				0x44
+#define SLV_IMR				0x48
+#define SLV_ICR				0x4c
+#define SLV_ISR				0x50
+#define SLV_INT_TM			BIT(20)
+#define SLV_INT_ERROR			BIT(19)
+#define SLV_INT_EVENT_UP		BIT(18)
+#define SLV_INT_HJ_DONE			BIT(17)
+#define SLV_INT_MR_DONE			BIT(16)
+#define SLV_INT_DA_UPD			BIT(15)
+#define SLV_INT_SDR_FAIL		BIT(14)
+#define SLV_INT_DDR_FAIL		BIT(13)
+#define SLV_INT_M_RD_ABORT		BIT(12)
+#define SLV_INT_DDR_RX_THR		BIT(11)
+#define SLV_INT_DDR_TX_THR		BIT(10)
+#define SLV_INT_SDR_RX_THR		BIT(9)
+#define SLV_INT_SDR_TX_THR		BIT(8)
+#define SLV_INT_DDR_RX_UNF		BIT(7)
+#define SLV_INT_DDR_TX_OVF		BIT(6)
+#define SLV_INT_SDR_RX_UNF		BIT(5)
+#define SLV_INT_SDR_TX_OVF		BIT(4)
+#define SLV_INT_DDR_RD_COMP		BIT(3)
+#define SLV_INT_DDR_WR_COMP		BIT(2)
+#define SLV_INT_SDR_RD_COMP		BIT(1)
+#define SLV_INT_SDR_WR_COMP		BIT(0)
+
+#define SLV_STATUS0			0x54
+#define SLV_STATUS0_REG_ADDR(s)		(((s) & GENMASK(23, 16)) >> 16)
+#define SLV_STATUS0_XFRD_BYTES(s)	((s) & GENMASK(15, 0))
+
+#define SLV_STATUS1			0x58
+#define SLV_STATUS1_AS(s)		(((s) & GENMASK(21, 20)) >> 20)
+#define SLV_STATUS1_VEN_TM		BIT(19)
+#define SLV_STATUS1_HJ_DIS		BIT(18)
+#define SLV_STATUS1_MR_DIS		BIT(17)
+#define SLV_STATUS1_PROT_ERR		BIT(16)
+#define SLV_STATUS1_DA(x)		(((s) & GENMASK(15, 9)) >> 9)
+#define SLV_STATUS1_HAS_DA		BIT(8)
+#define SLV_STATUS1_DDR_RX_FULL		BIT(7)
+#define SLV_STATUS1_DDR_TX_FULL		BIT(6)
+#define SLV_STATUS1_DDR_RX_EMPTY	BIT(5)
+#define SLV_STATUS1_DDR_TX_EMPTY	BIT(4)
+#define SLV_STATUS1_SDR_RX_FULL		BIT(3)
+#define SLV_STATUS1_SDR_TX_FULL		BIT(2)
+#define SLV_STATUS1_SDR_RX_EMPTY	BIT(1)
+#define SLV_STATUS1_SDR_TX_EMPTY	BIT(0)
+
+#define CMD0_FIFO			0x60
+#define CMD0_FIFO_IS_DDR		BIT(31)
+#define CMD0_FIFO_IS_CCC		BIT(30)
+#define CMD0_FIFO_BCH			BIT(29)
+#define XMIT_BURST_STATIC_SUBADDR	0
+#define XMIT_SINGLE_INC_SUBADDR		1
+#define XMIT_SINGLE_STATIC_SUBADDR	2
+#define XMIT_BURST_WITHOUT_SUBADDR	3
+#define CMD0_FIFO_PRIV_XMIT_MODE(m)	((m) << 27)
+#define CMD0_FIFO_SBCA			BIT(26)
+#define CMD0_FIFO_RSBC			BIT(25)
+#define CMD0_FIFO_IS_10B		BIT(24)
+#define CMD0_FIFO_PL_LEN(l)		((l) << 12)
+#define CMD0_FIFO_PL_LEN_MAX		4095
+#define CMD0_FIFO_DEV_ADDR(a)		((a) << 1)
+#define CMD0_FIFO_RNW			BIT(0)
+
+#define CMD1_FIFO			0x64
+#define CMD1_FIFO_CMDID(id)		((id) << 24)
+#define CMD1_FIFO_CSRADDR(a)		(a)
+#define CMD1_FIFO_CCC(id)		(id)
+
+#define TX_FIFO				0x68
+
+#define IMD_CMD0			0x70
+#define IMD_CMD0_PL_LEN(l)		((l) << 12)
+#define IMD_CMD0_DEV_ADDR(a)		((a) << 1)
+#define IMD_CMD0_RNW			BIT(0)
+
+#define IMD_CMD1			0x74
+#define IMD_CMD1_CCC(id)		(id)
+
+#define IMD_DATA			0x78
+#define RX_FIFO				0x80
+#define IBI_DATA_FIFO			0x84
+#define SLV_DDR_TX_FIFO			0x88
+#define SLV_DDR_RX_FIFO			0x8c
+
+#define CMD_IBI_THR_CTRL		0x90
+#define IBIR_THR(t)			((t) << 24)
+#define CMDR_THR(t)			((t) << 16)
+#define IBI_THR(t)			((t) << 8)
+#define CMD_THR(t)			(t)
+
+#define TX_RX_THR_CTRL			0x94
+#define RX_THR(t)			((t) << 16)
+#define TX_THR(t)			(t)
+
+#define SLV_DDR_TX_RX_THR_CTRL		0x98
+#define SLV_DDR_RX_THR(t)		((t) << 16)
+#define SLV_DDR_TX_THR(t)		(t)
+
+#define FLUSH_CTRL			0x9c
+#define FLUSH_IBI_RESP			BIT(23)
+#define FLUSH_CMD_RESP			BIT(22)
+#define FLUSH_SLV_DDR_RX_FIFO		BIT(22)
+#define FLUSH_SLV_DDR_TX_FIFO		BIT(21)
+#define FLUSH_IMM_FIFO			BIT(20)
+#define FLUSH_IBI_FIFO			BIT(19)
+#define FLUSH_RX_FIFO			BIT(18)
+#define FLUSH_TX_FIFO			BIT(17)
+#define FLUSH_CMD_FIFO			BIT(16)
+
+#define TTO_PRESCL_CTRL0		0xb0
+#define TTO_PRESCL_CTRL0_DIVB(x)	((x) << 16)
+#define TTO_PRESCL_CTRL0_DIVA(x)	(x)
+
+#define TTO_PRESCL_CTRL1		0xb4
+#define TTO_PRESCL_CTRL1_DIVB(x)	((x) << 16)
+#define TTO_PRESCL_CTRL1_DIVA(x)	(x)
+
+#define DEVS_CTRL			0xb8
+#define DEVS_CTRL_DEV_CLR_SHIFT		16
+#define DEVS_CTRL_DEV_CLR_ALL		GENMASK(31, 16)
+#define DEVS_CTRL_DEV_CLR(dev)		BIT(16 + (dev))
+#define DEVS_CTRL_DEV_ACTIVE(dev)	BIT(dev)
+#define DEVS_CTRL_DEVS_ACTIVE_MASK	GENMASK(15, 0)
+#define MAX_DEVS			16
+
+#define DEV_ID_RR0(d)			(0xc0 + ((d) * 0x10))
+#define DEV_ID_RR0_LVR_EXT_ADDR		BIT(11)
+#define DEV_ID_RR0_HDR_CAP		BIT(10)
+#define DEV_ID_RR0_IS_I3C		BIT(9)
+#define DEV_ID_RR0_DEV_ADDR_MASK	(GENMASK(6, 0) | GENMASK(15, 13))
+#define DEV_ID_RR0_SET_DEV_ADDR(a)	(((a) & GENMASK(6, 0)) |	\
+					 (((a) & GENMASK(9, 7)) << 6))
+#define DEV_ID_RR0_GET_DEV_ADDR(x)	((((x) >> 1) & GENMASK(6, 0)) |	\
+					 (((x) >> 6) & GENMASK(9, 7)))
+
+#define DEV_ID_RR1(d)			(0xc4 + ((d) * 0x10))
+#define DEV_ID_RR1_PID_MSB(pid)		(pid)
+
+#define DEV_ID_RR2(d)			(0xc8 + ((d) * 0x10))
+#define DEV_ID_RR2_PID_LSB(pid)		((pid) << 16)
+#define DEV_ID_RR2_BCR(bcr)		((bcr) << 8)
+#define DEV_ID_RR2_DCR(dcr)		(dcr)
+#define DEV_ID_RR2_LVR(lvr)		(lvr)
+
+#define SIR_MAP(x)			(0x180 + ((x) * 4))
+#define SIR_MAP_DEV_REG(d)		SIR_MAP((d) / 2)
+#define SIR_MAP_DEV_SHIFT(d, fs)	((fs) + (((d) % 2) ? 16 : 0))
+#define SIR_MAP_DEV_CONF_MASK(d)	(GENMASK(15, 0) << (((d) % 2) ? 16 : 0))
+#define SIR_MAP_DEV_CONF(d, c)		((c) << (((d) % 2) ? 16 : 0))
+#define DEV_ROLE_SLAVE			0
+#define DEV_ROLE_MASTER			1
+#define SIR_MAP_DEV_ROLE(role)		((role) << 14)
+#define SIR_MAP_DEV_SLOW		BIT(13)
+#define SIR_MAP_DEV_PL(l)		((l) << 8)
+#define SIR_MAP_PL_MAX			GENMASK(4, 0)
+#define SIR_MAP_DEV_DA(a)		((a) << 1)
+#define SIR_MAP_DEV_ACK			BIT(0)
+
+#define GPIR_WORD(x)			(0x200 + ((x) * 4))
+#define GPI_REG(val, id)		\
+	(((val) >> (((id) % 4) * 8)) & GENMASK(7, 0))
+
+#define GPOR_WORD(x)			(0x220 + ((x) * 4))
+#define GPO_REG(val, id)		\
+	(((val) >> (((id) % 4) * 8)) & GENMASK(7, 0))
+
+#define ASF_INT_STATUS			0x300
+#define ASF_INT_RAW_STATUS		0x304
+#define ASF_INT_MASK			0x308
+#define ASF_INT_TEST			0x30c
+#define ASF_INT_FATAL_SELECT		0x310
+#define ASF_INTEGRITY_ERR		BIT(6)
+#define ASF_PROTOCOL_ERR		BIT(5)
+#define ASF_TRANS_TIMEOUT_ERR		BIT(4)
+#define ASF_CSR_ERR			BIT(3)
+#define ASF_DAP_ERR			BIT(2)
+#define ASF_SRAM_UNCORR_ERR		BIT(1)
+#define ASF_SRAM_CORR_ERR		BIT(0)
+
+#define ASF_SRAM_CORR_FAULT_STATUS	0x320
+#define ASF_SRAM_UNCORR_FAULT_STATUS	0x324
+#define ASF_SRAM_CORR_FAULT_INSTANCE(x)	((x) >> 24)
+#define ASF_SRAM_CORR_FAULT_ADDR(x)	((x) & GENMASK(23, 0))
+
+#define ASF_SRAM_FAULT_STATS		0x328
+#define ASF_SRAM_FAULT_UNCORR_STATS(x)	((x) >> 16)
+#define ASF_SRAM_FAULT_CORR_STATS(x)	((x) & GENMASK(15, 0))
+
+#define ASF_TRANS_TOUT_CTRL		0x330
+#define ASF_TRANS_TOUT_EN		BIT(31)
+#define ASF_TRANS_TOUT_VAL(x)	(x)
+
+#define ASF_TRANS_TOUT_FAULT_MASK	0x334
+#define ASF_TRANS_TOUT_FAULT_STATUS	0x338
+#define ASF_TRANS_TOUT_FAULT_APB	BIT(3)
+#define ASF_TRANS_TOUT_FAULT_SCL_LOW	BIT(2)
+#define ASF_TRANS_TOUT_FAULT_SCL_HIGH	BIT(1)
+#define ASF_TRANS_TOUT_FAULT_FSCL_HIGH	BIT(0)
+
+#define ASF_PROTO_FAULT_MASK		0x340
+#define ASF_PROTO_FAULT_STATUS		0x344
+#define ASF_PROTO_FAULT_SLVSDR_RD_ABORT	BIT(31)
+#define ASF_PROTO_FAULT_SLVDDR_FAIL	BIT(30)
+#define ASF_PROTO_FAULT_S(x)		BIT(16 + (x))
+#define ASF_PROTO_FAULT_MSTSDR_RD_ABORT	BIT(15)
+#define ASF_PROTO_FAULT_MSTDDR_FAIL	BIT(14)
+#define ASF_PROTO_FAULT_M(x)		BIT(x)
+
+struct cdns_i3c_master_caps {
+	u32 cmdfifodepth;
+	u32 cmdrfifodepth;
+	u32 txfifodepth;
+	u32 rxfifodepth;
+	u32 ibirfifodepth;
+};
+
+struct cdns_i3c_cmd {
+	u32 cmd0;
+	u32 cmd1;
+	u32 tx_len;
+	const void *tx_buf;
+	u32 rx_len;
+	void *rx_buf;
+	u32 error;
+};
+
+struct cdns_i3c_xfer {
+	struct list_head node;
+	struct completion comp;
+	int ret;
+	unsigned int ncmds;
+	struct cdns_i3c_cmd cmds[0];
+};
+
+struct cdns_i3c_master {
+	struct work_struct hj_work;
+	struct i3c_master_controller base;
+	u32 free_rr_slots;
+	unsigned int maxdevs;
+	struct {
+		unsigned int num_slots;
+		struct i3c_device **slots;
+		spinlock_t lock;
+	} ibi;
+	struct {
+		struct list_head list;
+		struct cdns_i3c_xfer *cur;
+		spinlock_t lock;
+	} xferqueue;
+	void __iomem *regs;
+	struct clk *sysclk;
+	struct clk *pclk;
+	struct cdns_i3c_master_caps caps;
+	unsigned long i3c_scl_lim;
+};
+
+static inline struct cdns_i3c_master *
+to_cdns_i3c_master(struct i3c_master_controller *master)
+{
+	return container_of(master, struct cdns_i3c_master, base);
+}
+
+static void cdns_i3c_master_wr_to_tx_fifo(struct cdns_i3c_master *master,
+					  const u8 *bytes, int nbytes)
+{
+	int i, j;
+
+	for (i = 0; i < nbytes; i += 4) {
+		u32 data = 0;
+
+		for (j = 0; j < 4 && (i + j) < nbytes; j++)
+			data |= (u32)bytes[i + j] << (j * 8);
+
+		writel(data, master->regs + TX_FIFO);
+	}
+}
+
+static void cdns_i3c_master_rd_from_rx_fifo(struct cdns_i3c_master *master,
+					    u8 *bytes, int nbytes)
+{
+	int i, j;
+
+	for (i = 0; i < nbytes; i += 4) {
+		u32 data;
+
+		data = readl(master->regs + RX_FIFO);
+
+		for (j = 0; j < 4 && (i + j) < nbytes; j++)
+			bytes[i + j] = data >> (j * 8);
+	}
+}
+
+static bool cdns_i3c_master_supports_ccc_cmd(struct i3c_master_controller *m,
+					     const struct i3c_ccc_cmd *cmd)
+{
+	if (cmd->ndests > 1)
+		return false;
+
+	switch (cmd->id) {
+	case I3C_CCC_ENEC(true):
+	case I3C_CCC_ENEC(false):
+	case I3C_CCC_DISEC(true):
+	case I3C_CCC_DISEC(false):
+	case I3C_CCC_ENTAS(0, true):
+	case I3C_CCC_ENTAS(0, false):
+	case I3C_CCC_RSTDAA(true):
+	case I3C_CCC_RSTDAA(false):
+	case I3C_CCC_ENTDAA:
+	case I3C_CCC_SETMWL(true):
+	case I3C_CCC_SETMWL(false):
+	case I3C_CCC_SETMRL(true):
+	case I3C_CCC_SETMRL(false):
+	case I3C_CCC_DEFSLVS:
+	case I3C_CCC_ENTHDR(0):
+	case I3C_CCC_SETDASA:
+	case I3C_CCC_SETNEWDA:
+	case I3C_CCC_GETMWL:
+	case I3C_CCC_GETMRL:
+	case I3C_CCC_GETPID:
+	case I3C_CCC_GETBCR:
+	case I3C_CCC_GETDCR:
+	case I3C_CCC_GETSTATUS:
+	case I3C_CCC_GETACCMST:
+	case I3C_CCC_GETMXDS:
+	case I3C_CCC_GETHDRCAP:
+		return true;
+	default:
+		break;
+	}
+
+	return false;
+}
+
+static int cdns_i3c_master_disable(struct cdns_i3c_master *master)
+{
+	u32 status;
+
+	writel(readl(master->regs + CTRL) & ~CTRL_DEV_EN, master->regs + CTRL);
+
+	return readl_poll_timeout(master->regs + MST_STATUS0, status,
+				  status & MST_STATUS0_IDLE, 10, 1000000);
+}
+
+static void cdns_i3c_master_enable(struct cdns_i3c_master *master)
+{
+	writel(readl(master->regs + CTRL) | CTRL_DEV_EN, master->regs + CTRL);
+}
+
+static struct cdns_i3c_xfer *
+cdns_i3c_master_alloc_xfer(struct cdns_i3c_master *master, unsigned int ncmds)
+{
+	struct cdns_i3c_xfer *xfer;
+
+	xfer = kzalloc(sizeof(*xfer) + (ncmds * sizeof(*xfer->cmds)),
+		       GFP_KERNEL);
+	if (!xfer)
+		return NULL;
+
+	INIT_LIST_HEAD(&xfer->node);
+	xfer->ncmds = ncmds;
+	xfer->ret = -ETIMEDOUT;
+
+	return xfer;
+}
+
+static void cdns_i3c_master_free_xfer(struct cdns_i3c_xfer *xfer)
+{
+	kfree(xfer);
+}
+
+static void cdns_i3c_master_start_xfer_locked(struct cdns_i3c_master *master)
+{
+	struct cdns_i3c_xfer *xfer = master->xferqueue.cur;
+	unsigned int i;
+
+	if (!xfer)
+		return;
+
+	writel(MST_INT_CMDD_EMP, master->regs + MST_ICR);
+	for (i = 0; i < xfer->ncmds; i++) {
+		struct cdns_i3c_cmd *cmd = &xfer->cmds[i];
+
+		cdns_i3c_master_wr_to_tx_fifo(master, cmd->tx_buf,
+					      cmd->tx_len);
+	}
+
+	for (i = 0; i < xfer->ncmds; i++) {
+		struct cdns_i3c_cmd *cmd = &xfer->cmds[i];
+
+		writel(cmd->cmd1 | CMD1_FIFO_CMDID(i),
+		       master->regs + CMD1_FIFO);
+		writel(cmd->cmd0, master->regs + CMD0_FIFO);
+	}
+
+	writel(readl(master->regs + CTRL) | CTRL_MCS,
+	       master->regs + CTRL);
+	writel(MST_INT_CMDD_EMP, master->regs + MST_IER);
+}
+
+static void cdns_i3c_master_end_xfer_locked(struct cdns_i3c_master *master,
+					    u32 isr)
+{
+	struct cdns_i3c_xfer *xfer = master->xferqueue.cur;
+	int i, ret = 0;
+	u32 status0;
+
+	if (!xfer)
+		return;
+
+	if (!(isr & MST_INT_CMDD_EMP))
+		return;
+
+	writel(MST_INT_CMDD_EMP, master->regs + MST_IDR);
+
+	for (status0 = readl(master->regs + MST_STATUS0);
+	     !(status0 & MST_STATUS0_CMDR_EMP);
+	     status0 = readl(master->regs + MST_STATUS0)) {
+		struct cdns_i3c_cmd *cmd;
+		u32 cmdr, rx_len, id;
+
+		cmdr = readl(master->regs + CMDR);
+		id = CMDR_CMDID(cmdr);
+		if (id == CMDR_CMDID_HJACK_DISEC ||
+		    id == CMDR_CMDID_HJACK_ENTDAA ||
+		    WARN_ON(id >= xfer->ncmds))
+			continue;
+
+		cmd = &xfer->cmds[CMDR_CMDID(cmdr)];
+		rx_len = min_t(u32, CMDR_XFER_BYTES(cmdr), cmd->rx_len);
+		cdns_i3c_master_rd_from_rx_fifo(master, cmd->rx_buf, rx_len);
+		cmd->error = CMDR_ERROR(cmdr);
+	}
+
+	for (i = 0; i < xfer->ncmds; i++) {
+		switch (xfer->cmds[i].error) {
+		case CMDR_NO_ERROR:
+			break;
+
+		case CMDR_DDR_PREAMBLE_ERROR:
+		case CMDR_DDR_PARITY_ERROR:
+		case CMDR_M0_ERROR:
+		case CMDR_M1_ERROR:
+		case CMDR_M2_ERROR:
+		case CMDR_MST_ABORT:
+		case CMDR_NACK_RESP:
+		case CMDR_DDR_DROPPED:
+			ret = -EIO;
+			break;
+
+		case CMDR_DDR_RX_FIFO_OVF:
+		case CMDR_DDR_TX_FIFO_UNF:
+			ret = -ENOSPC;
+			break;
+
+		case CMDR_INVALID_DA:
+		default:
+			ret = -EINVAL;
+			break;
+		}
+	}
+
+	xfer->ret = ret;
+	complete(&xfer->comp);
+
+	xfer = list_first_entry_or_null(&master->xferqueue.list,
+					struct cdns_i3c_xfer, node);
+	if (xfer)
+		list_del_init(&xfer->node);
+
+	master->xferqueue.cur = xfer;
+	cdns_i3c_master_start_xfer_locked(master);
+}
+
+static void cdns_i3c_master_queue_xfer(struct cdns_i3c_master *master,
+				       struct cdns_i3c_xfer *xfer)
+{
+	unsigned long flags;
+
+	init_completion(&xfer->comp);
+	spin_lock_irqsave(&master->xferqueue.lock, flags);
+	if (master->xferqueue.cur) {
+		list_add_tail(&xfer->node, &master->xferqueue.list);
+	} else {
+		master->xferqueue.cur = xfer;
+		cdns_i3c_master_start_xfer_locked(master);
+	}
+	spin_unlock_irqrestore(&master->xferqueue.lock, flags);
+}
+
+static void cdns_i3c_master_unqueue_xfer(struct cdns_i3c_master *master,
+					 struct cdns_i3c_xfer *xfer)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&master->xferqueue.lock, flags);
+	if (master->xferqueue.cur == xfer) {
+		u32 status;
+
+		writel(readl(master->regs + CTRL) & ~CTRL_DEV_EN,
+		       master->regs + CTRL);
+		readl_poll_timeout_atomic(master->regs + MST_STATUS0, status,
+					  status & MST_STATUS0_IDLE, 10,
+					  1000000);
+		master->xferqueue.cur = NULL;
+		writel(FLUSH_RX_FIFO | FLUSH_TX_FIFO | FLUSH_CMD_FIFO |
+		       FLUSH_CMD_RESP,
+		       master->regs + FLUSH_CTRL);
+		writel(MST_INT_CMDD_EMP, master->regs + MST_IDR);
+		writel(readl(master->regs + CTRL) | CTRL_DEV_EN,
+		       master->regs + CTRL);
+	} else {
+		list_del_init(&xfer->node);
+	}
+	spin_unlock_irqrestore(&master->xferqueue.lock, flags);
+}
+
+static int cdns_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
+					struct i3c_ccc_cmd *cmd)
+{
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	struct cdns_i3c_xfer *xfer;
+	struct cdns_i3c_cmd *ccmd;
+	int ret;
+
+	xfer = cdns_i3c_master_alloc_xfer(master, 1);
+	if (!xfer)
+		return -ENOMEM;
+
+	ccmd = xfer->cmds;
+	ccmd->cmd1 = CMD1_FIFO_CCC(cmd->id);
+	ccmd->cmd0 = CMD0_FIFO_IS_CCC |
+		     CMD0_FIFO_PL_LEN(cmd->dests[0].payload.len);
+
+	if (cmd->id & I3C_CCC_DIRECT)
+		ccmd->cmd0 |= CMD0_FIFO_DEV_ADDR(cmd->dests[0].addr);
+
+	if (cmd->rnw) {
+		ccmd->cmd0 |= CMD0_FIFO_RNW;
+		ccmd->rx_buf = cmd->dests[0].payload.data;
+		ccmd->rx_len = cmd->dests[0].payload.len;
+	} else {
+		ccmd->tx_buf = cmd->dests[0].payload.data;
+		ccmd->tx_len = cmd->dests[0].payload.len;
+	}
+
+	cdns_i3c_master_queue_xfer(master, xfer);
+	if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000)))
+		cdns_i3c_master_unqueue_xfer(master, xfer);
+
+	ret = xfer->ret;
+	cdns_i3c_master_free_xfer(xfer);
+
+	return ret;
+}
+
+static int cdns_i3c_master_priv_xfers(struct i3c_device *dev,
+				      const struct i3c_priv_xfer *xfers,
+				      int nxfers)
+{
+	struct i3c_master_controller *m = i3c_device_get_master(dev);
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	int txslots = 0, rxslots = 0, i, ret;
+	struct cdns_i3c_xfer *cdns_xfer;
+
+	for (i = 0; i < nxfers; i++) {
+		if (xfers[i].len > CMD0_FIFO_PL_LEN_MAX)
+			return -ENOTSUPP;
+	}
+
+	if (!nxfers)
+		return 0;
+
+	if (nxfers > master->caps.cmdfifodepth ||
+	    nxfers > master->caps.cmdrfifodepth)
+		return -ENOTSUPP;
+
+	/*
+	 * First make sure that all transactions (block of transfers separated
+	 * by a STOP marker) fit in the FIFOs.
+	 */
+	for (i = 0; i < nxfers; i++) {
+		if (xfers[i].rnw)
+			rxslots += DIV_ROUND_UP(xfers[i].len, 4);
+		else
+			txslots += DIV_ROUND_UP(xfers[i].len, 4);
+	}
+
+	if (rxslots > master->caps.rxfifodepth ||
+	    txslots > master->caps.txfifodepth)
+		return -ENOTSUPP;
+
+	cdns_xfer = cdns_i3c_master_alloc_xfer(master, nxfers);
+	if (!cdns_xfer)
+		return -ENOMEM;
+
+	for (i = 0; i < nxfers; i++) {
+		struct cdns_i3c_cmd *ccmd = &cdns_xfer->cmds[i];
+		u32 pl_len = xfers[i].len;
+
+		ccmd->cmd0 = CMD0_FIFO_DEV_ADDR(dev->info.dyn_addr) |
+			CMD0_FIFO_PRIV_XMIT_MODE(XMIT_BURST_WITHOUT_SUBADDR);
+
+		if (xfers[i].rnw) {
+			ccmd->cmd0 |= CMD0_FIFO_RNW;
+			ccmd->rx_buf = xfers[i].data.in;
+			ccmd->rx_len = xfers[i].len;
+			pl_len++;
+		} else {
+			ccmd->tx_buf = xfers[i].data.out;
+			ccmd->tx_len = xfers[i].len;
+		}
+
+		ccmd->cmd0 |= CMD0_FIFO_PL_LEN(pl_len);
+
+		if (i < nxfers - 1)
+			ccmd->cmd0 |= CMD0_FIFO_RSBC;
+
+		if (!i)
+			ccmd->cmd0 |= CMD0_FIFO_BCH;
+	}
+
+	cdns_i3c_master_queue_xfer(master, cdns_xfer);
+	if (!wait_for_completion_timeout(&cdns_xfer->comp,
+					 msecs_to_jiffies(1000)))
+		cdns_i3c_master_unqueue_xfer(master, cdns_xfer);
+
+	ret = cdns_xfer->ret;
+	cdns_i3c_master_free_xfer(cdns_xfer);
+
+	return ret;
+}
+
+static int cdns_i3c_master_i2c_xfers(struct i2c_device *dev,
+				     const struct i2c_msg *xfers, int nxfers)
+{
+	struct i3c_master_controller *m = i2c_device_get_master(dev);
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	unsigned int nrxwords = 0, ntxwords = 0;
+	struct cdns_i3c_xfer *xfer;
+	int i, ret = 0;
+
+	if (nxfers > master->caps.cmdfifodepth)
+		return -ENOTSUPP;
+
+	for (i = 0; i < nxfers; i++) {
+		if (xfers[i].len > CMD0_FIFO_PL_LEN_MAX)
+			return -ENOTSUPP;
+
+		if (xfers[i].flags & I2C_M_RD)
+			nrxwords += DIV_ROUND_UP(xfers[i].len, 4);
+		else
+			ntxwords += DIV_ROUND_UP(xfers[i].len, 4);
+	}
+
+	if (ntxwords > master->caps.txfifodepth ||
+	    nrxwords > master->caps.rxfifodepth)
+		return -ENOTSUPP;
+
+	xfer = cdns_i3c_master_alloc_xfer(master, nxfers);
+	if (!xfer)
+		return -ENOMEM;
+
+	for (i = 0; i < nxfers; i++) {
+		struct cdns_i3c_cmd *ccmd = &xfer->cmds[0];
+
+		ccmd->cmd0 = CMD0_FIFO_DEV_ADDR(xfers[i].addr) |
+			CMD0_FIFO_PL_LEN(xfers[i].len) |
+			CMD0_FIFO_PRIV_XMIT_MODE(XMIT_BURST_WITHOUT_SUBADDR);
+
+		if (xfers[i].flags & I2C_M_TEN)
+			ccmd->cmd0 |= CMD0_FIFO_IS_10B;
+
+		if (xfers[i].flags & I2C_M_RD) {
+			ccmd->cmd0 |= CMD0_FIFO_RNW;
+			ccmd->rx_buf = xfers[i].buf;
+			ccmd->rx_len = xfers[i].len;
+		} else {
+			ccmd->tx_buf = xfers[i].buf;
+			ccmd->tx_len = xfers[i].len;
+		}
+	}
+
+	cdns_i3c_master_queue_xfer(master, xfer);
+	if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000)))
+		cdns_i3c_master_unqueue_xfer(master, xfer);
+
+	ret = xfer->ret;
+	cdns_i3c_master_free_xfer(xfer);
+
+	return ret;
+}
+
+static u32 cdns_i3c_master_i2c_funcs(struct i3c_master_controller *m)
+{
+	return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR;
+}
+
+struct cdns_i3c_i2c_dev_data {
+	u16 id;
+	s16 ibi;
+	struct i3c_generic_ibi_pool *ibi_pool;
+};
+
+static u32 prepare_rr0_dev_address(u32 addr)
+{
+	u32 ret = (addr << 1) & 0xff;
+
+	/* RR0[7:1] = addr[6:0] */
+	ret |= (addr & GENMASK(6, 0)) << 1;
+
+	/* RR0[15:13] = addr[9:7] */
+	ret |= (addr & GENMASK(9, 7)) << 6;
+
+	/* RR0[0] = ~XOR(addr[6:0]) */
+	if (!(hweight8(addr & 0x7f) & 1))
+		ret |= 1;
+
+	return ret;
+}
+
+static void cdns_i3c_master_upd_i3c_addr(struct i3c_device *dev)
+{
+	struct i3c_master_controller *m = i3c_device_get_master(dev);
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	struct cdns_i3c_i2c_dev_data *data = i3c_device_get_master_data(dev);
+	u32 rr;
+
+	rr = prepare_rr0_dev_address(dev->info.dyn_addr ?
+				     dev->info.dyn_addr :
+				     dev->info.static_addr);
+	writel(DEV_ID_RR0_IS_I3C | rr, master->regs + DEV_ID_RR0(data->id));
+}
+
+static int cdns_i3c_master_get_rr_slot(struct cdns_i3c_master *master,
+				       u8 dyn_addr)
+{
+	u32 activedevs, rr;
+	int i;
+
+	if (!dyn_addr) {
+		if (!master->free_rr_slots)
+			return -ENOSPC;
+
+		return ffs(master->free_rr_slots) - 1;
+	}
+
+	activedevs = readl(master->regs + DEVS_CTRL) &
+		     DEVS_CTRL_DEVS_ACTIVE_MASK;
+
+	for (i = 1; i <= master->maxdevs; i++) {
+		if (!(BIT(i) & activedevs))
+			continue;
+
+		rr = readl(master->regs + DEV_ID_RR0(i));
+		if (!(rr & DEV_ID_RR0_IS_I3C) ||
+		    DEV_ID_RR0_GET_DEV_ADDR(rr) != dyn_addr)
+			continue;
+
+		return i;
+	}
+
+	return -EINVAL;
+}
+
+static void cdns_i3c_master_reattach_i3c_dev(struct i3c_device *dev,
+					     u8 old_dyn_addr)
+{
+	struct i3c_master_controller *m = i3c_device_get_master(dev);
+
+	cdns_i3c_master_upd_i3c_addr(dev);
+	if (!old_dyn_addr)
+		return;
+
+	/* Now, make sure we re-enable the IBI if needed. */
+	mutex_lock(&dev->ibi_lock);
+	if (dev->ibi && dev->ibi->enabled) {
+		int ret;
+
+		ret = i3c_master_enec_locked(m, dev->info.dyn_addr,
+					     I3C_CCC_EVENT_SIR);
+		if (ret)
+			dev_err(&dev->dev, "Could not re-enable IBIs");
+	}
+	mutex_unlock(&dev->ibi_lock);
+}
+
+static int cdns_i3c_master_attach_i3c_dev(struct i3c_device *dev)
+{
+	struct i3c_master_controller *m = i3c_device_get_master(dev);
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	struct cdns_i3c_i2c_dev_data *data;
+	unsigned long max_fscl;
+	int slot;
+
+	slot = cdns_i3c_master_get_rr_slot(master, dev->info.dyn_addr);
+	if (slot < 0)
+		return slot;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->ibi = -1;
+	data->id = slot;
+	i3c_device_set_master_data(dev, data);
+	master->free_rr_slots &= ~BIT(slot);
+
+	if (!dev->info.dyn_addr) {
+		cdns_i3c_master_upd_i3c_addr(dev);
+		writel(readl(master->regs + DEVS_CTRL) |
+		       DEVS_CTRL_DEV_ACTIVE(data->id),
+		       master->regs + DEVS_CTRL);
+		return 0;
+	}
+
+	max_fscl = max(I3C_CCC_MAX_SDR_FSCL(dev->info.max_read_ds),
+		       I3C_CCC_MAX_SDR_FSCL(dev->info.max_write_ds));
+	switch (max_fscl) {
+	case I3C_SDR1_FSCL_8MHZ:
+		max_fscl = 8000000;
+		break;
+	case I3C_SDR2_FSCL_6MHZ:
+		max_fscl = 6000000;
+		break;
+	case I3C_SDR3_FSCL_4MHZ:
+		max_fscl = 4000000;
+		break;
+	case I3C_SDR4_FSCL_2MHZ:
+		max_fscl = 2000000;
+		break;
+	case I3C_SDR0_FSCL_MAX:
+	default:
+		max_fscl = 0;
+		break;
+	}
+
+	/* Update SCL limitation in I3C SDR mode. */
+	if (max_fscl &&
+	    (master->i3c_scl_lim > max_fscl || !master->i3c_scl_lim))
+		master->i3c_scl_lim = max_fscl;
+
+	return 0;
+}
+
+static void cdns_i3c_master_detach_i3c_dev(struct i3c_device *dev)
+{
+	struct i3c_master_controller *m = i3c_device_get_master(dev);
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	struct cdns_i3c_i2c_dev_data *data = i3c_device_get_master_data(dev);
+
+	writel(readl(master->regs + DEVS_CTRL) |
+	       DEVS_CTRL_DEV_CLR(data->id),
+	       master->regs + DEVS_CTRL);
+
+	i3c_device_set_master_data(dev, NULL);
+	master->free_rr_slots |= BIT(data->id);
+	kfree(data);
+}
+
+static int cdns_i3c_master_attach_i2c_dev(struct i2c_device *dev)
+{
+	struct i3c_master_controller *m = i2c_device_get_master(dev);
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	struct cdns_i3c_i2c_dev_data *data;
+	int slot;
+
+	slot = cdns_i3c_master_get_rr_slot(master, 0);
+	if (slot < 0)
+		return slot;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->id = slot;
+	master->free_rr_slots &= ~BIT(slot);
+	i2c_device_set_master_data(dev, data);
+
+	writel(prepare_rr0_dev_address(dev->info.addr) |
+	       (dev->info.flags & I2C_CLIENT_TEN ? DEV_ID_RR0_LVR_EXT_ADDR : 0),
+	       master->regs + DEV_ID_RR0(data->id));
+	writel(dev->lvr, master->regs + DEV_ID_RR2(data->id));
+	writel(readl(master->regs + DEVS_CTRL) |
+	       DEVS_CTRL_DEV_ACTIVE(data->id),
+	       master->regs + DEVS_CTRL);
+
+	return 0;
+}
+
+static void cdns_i3c_master_detach_i2c_dev(struct i2c_device *dev)
+{
+	struct i3c_master_controller *m = i2c_device_get_master(dev);
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	struct cdns_i3c_i2c_dev_data *data = i2c_device_get_master_data(dev);
+
+	writel(readl(master->regs + DEVS_CTRL) |
+	       DEVS_CTRL_DEV_CLR(data->id),
+	       master->regs + DEVS_CTRL);
+	master->free_rr_slots |= BIT(data->id);
+
+	i2c_device_set_master_data(dev, NULL);
+	kfree(data);
+}
+
+static void cdns_i3c_master_bus_cleanup(struct i3c_master_controller *m)
+{
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+
+	cdns_i3c_master_disable(master);
+}
+
+static void cdns_i3c_master_dev_rr_to_info(struct cdns_i3c_master *master,
+					   unsigned int slot,
+					   struct i3c_device_info *info)
+{
+	u32 rr;
+
+	memset(info, 0, sizeof(*info));
+	rr = readl(master->regs + DEV_ID_RR0(slot));
+	info->dyn_addr = DEV_ID_RR0_GET_DEV_ADDR(rr);
+	rr = readl(master->regs + DEV_ID_RR2(slot));
+	info->dcr = rr;
+	info->bcr = rr >> 8;
+	info->pid = rr >> 16;
+	info->pid |= (u64)readl(master->regs + DEV_ID_RR1(slot)) << 16;
+}
+
+static void cdns_i3c_master_upd_i3c_scl_lim(struct cdns_i3c_master *master)
+{
+	unsigned long i3c_lim_period, pres_step, ncycles;
+	u32 prescl1, ctrl;
+
+	pres_step = 1000000000UL / (master->base.bus->scl_rate.i3c * 4);
+
+	/* Configure PP_LOW to meet I3C slave limitations. */
+	prescl1 = readl(master->regs + PRESCL_CTRL1) &
+		  ~PRESCL_CTRL1_PP_LOW_MASK;
+	ctrl = readl(master->regs + CTRL);
+
+	i3c_lim_period = DIV_ROUND_UP(1000000000, master->i3c_scl_lim);
+	ncycles = DIV_ROUND_UP(i3c_lim_period, pres_step);
+	if (ncycles < 4)
+		ncycles = 0;
+	else
+		ncycles -= 4;
+
+	prescl1 |= PRESCL_CTRL1_PP_LOW(ncycles);
+
+	/* Disable I3C master before updating PRESCL_CTRL1. */
+	if (ctrl & CTRL_DEV_EN)
+		cdns_i3c_master_disable(master);
+
+	writel(prescl1, master->regs + PRESCL_CTRL1);
+
+	if (ctrl & CTRL_DEV_EN)
+		cdns_i3c_master_enable(master);
+}
+
+static int cdns_i3c_master_do_daa(struct i3c_master_controller *m)
+{
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	unsigned long old_i3c_scl_lim;
+	u32 olddevs, newdevs;
+	int ret, slot;
+	u8 addrs[MAX_DEVS] = { };
+	u8 last_addr = 0;
+
+	olddevs = readl(master->regs + DEVS_CTRL) & DEVS_CTRL_DEVS_ACTIVE_MASK;
+
+	/* Prepare RR slots before launching DAA. */
+	for (slot = 1; slot <= master->maxdevs; slot++) {
+		if (olddevs & BIT(slot))
+			continue;
+
+		ret = i3c_master_get_free_addr(m, last_addr + 1);
+		if (ret < 0)
+			return -ENOSPC;
+
+		last_addr = ret;
+		addrs[slot] = last_addr;
+		writel(prepare_rr0_dev_address(last_addr) | DEV_ID_RR0_IS_I3C,
+		       master->regs + DEV_ID_RR0(slot));
+		writel(0, master->regs + DEV_ID_RR1(slot));
+		writel(0, master->regs + DEV_ID_RR2(slot));
+	}
+
+	ret = i3c_master_entdaa_locked(&master->base);
+	if (ret)
+		return ret;
+
+	newdevs = readl(master->regs + DEVS_CTRL) & DEVS_CTRL_DEVS_ACTIVE_MASK;
+	newdevs &= ~olddevs;
+
+	/* Save the old limitation before add devices. */
+	old_i3c_scl_lim = master->i3c_scl_lim;
+
+	/*
+	 * Clear all retaining registers filled during DAA. We already
+	 * have the addressed assigned to them in the addrs array.
+	 */
+	for (slot = 1; slot <= master->maxdevs; slot++) {
+		if (newdevs & BIT(slot))
+			i3c_master_add_i3c_dev_locked(m, addrs[slot]);
+	}
+
+	/*
+	 * Clear slots that ended up not being used. Can be caused by I3C
+	 * device creation failure or when the I3C device was already known
+	 * by the system but with a different address (in this case the device
+	 * already has a slot and does not need a new one).
+	 */
+	writel(readl(master->regs + DEVS_CTRL) |
+	       master->free_rr_slots << DEVS_CTRL_DEV_CLR_SHIFT,
+	       master->regs + DEVS_CTRL);
+
+	i3c_master_defslvs_locked(&master->base);
+
+	/* Only update PRESCL_CTRL1 if the I3C SCL limitation has changed. */
+	if (old_i3c_scl_lim != master->i3c_scl_lim)
+		cdns_i3c_master_upd_i3c_scl_lim(master);
+
+	/* Unmask Hot-Join and Mastership request interrupts. */
+	i3c_master_enec_locked(m, I3C_BROADCAST_ADDR,
+			       I3C_CCC_EVENT_HJ | I3C_CCC_EVENT_MR);
+
+	return 0;
+}
+
+static int cdns_i3c_master_bus_init(struct i3c_master_controller *m)
+{
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	unsigned long pres_step, sysclk_rate, max_i2cfreq;
+	u32 ctrl, prescl0, prescl1, pres, low;
+	struct i3c_device_info info = { };
+	int ret, ncycles;
+
+	switch (m->bus->mode) {
+	case I3C_BUS_MODE_PURE:
+		ctrl = CTRL_PURE_BUS_MODE;
+		break;
+
+	case I3C_BUS_MODE_MIXED_FAST:
+		ctrl = CTRL_MIXED_FAST_BUS_MODE;
+		break;
+
+	case I3C_BUS_MODE_MIXED_SLOW:
+		ctrl = CTRL_MIXED_SLOW_BUS_MODE;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	sysclk_rate = clk_get_rate(master->sysclk);
+	if (!sysclk_rate)
+		return -EINVAL;
+
+	pres = DIV_ROUND_UP(sysclk_rate, (m->bus->scl_rate.i3c * 4)) - 1;
+	if (pres > PRESCL_CTRL0_MAX)
+		return -ERANGE;
+
+	m->bus->scl_rate.i3c = sysclk_rate / ((pres + 1) * 4);
+
+	prescl0 = PRESCL_CTRL0_I3C(pres);
+
+	low = ((I3C_BUS_TLOW_OD_MIN_NS * sysclk_rate) / (pres + 1)) - 2;
+	prescl1 = PRESCL_CTRL1_OD_LOW(low);
+
+	max_i2cfreq = m->bus->scl_rate.i2c;
+
+	pres = (sysclk_rate / (max_i2cfreq * 5)) - 1;
+	if (pres > PRESCL_CTRL0_MAX)
+		return -ERANGE;
+
+	m->bus->scl_rate.i2c = sysclk_rate / ((pres + 1) * 5);
+
+	prescl0 |= PRESCL_CTRL0_I2C(pres);
+	writel(prescl0, master->regs + PRESCL_CTRL0);
+
+	/* Calculate OD and PP low. */
+	pres_step = 1000000000 / (m->bus->scl_rate.i3c * 4);
+	ncycles = DIV_ROUND_UP(I3C_BUS_TLOW_OD_MIN_NS, pres_step) - 2;
+	if (ncycles < 0)
+		ncycles = 0;
+	prescl1 = PRESCL_CTRL1_OD_LOW(ncycles);
+	writel(prescl1, master->regs + PRESCL_CTRL1);
+
+	/* Get an address for the master. */
+	ret = i3c_master_get_free_addr(m, 0);
+	if (ret < 0)
+		return ret;
+
+	writel(prepare_rr0_dev_address(ret) | DEV_ID_RR0_IS_I3C,
+	       master->regs + DEV_ID_RR0(0));
+
+	cdns_i3c_master_dev_rr_to_info(master, 0, &info);
+	if (info.bcr & I3C_BCR_HDR_CAP)
+		info.hdr_cap = I3C_CCC_HDR_MODE(I3C_HDR_DDR);
+
+	ret = i3c_master_set_info(&master->base, &info);
+	if (ret)
+		return ret;
+
+	/*
+	 * Enable Hot-Join, and, when a Hot-Join request happens, disable all
+	 * events coming from this device.
+	 *
+	 * We will issue ENTDAA afterwards from the threaded IRQ handler.
+	 */
+	ctrl |= CTRL_HJ_ACK | CTRL_HJ_DISEC | CTRL_HALT_EN | CTRL_MCS_EN;
+	writel(ctrl, master->regs + CTRL);
+
+	cdns_i3c_master_enable(master);
+
+	return 0;
+}
+
+static void cdns_i3c_master_handle_ibi(struct cdns_i3c_master *master,
+				       u32 ibir)
+{
+	struct cdns_i3c_i2c_dev_data *data;
+	bool data_consumed = false;
+	struct i3c_ibi_slot *slot;
+	u32 id = IBIR_SLVID(ibir);
+	struct i3c_device *dev;
+	int i, j;
+	u8 *buf;
+
+	/*
+	 * FIXME: maybe we should report the FIFO OVF errors to the upper
+	 * layer.
+	 */
+	if (id >= master->ibi.num_slots || (ibir & IBIR_ERROR))
+		goto out;
+
+	dev = master->ibi.slots[id];
+	spin_lock(&master->ibi.lock);
+
+	data = i3c_device_get_master_data(dev);
+	slot = i3c_generic_ibi_get_free_slot(data->ibi_pool);
+	if (!slot)
+		goto out_unlock;
+
+	buf = slot->data;
+
+	for (i = 0; i < IBIR_XFER_BYTES(ibir); i += 4) {
+		u32 tmp = readl(master->regs + IBI_DATA_FIFO);
+
+		for (j = 0; j < 4 && i + j < dev->ibi->max_payload_len; j++)
+			buf[i + j] = tmp >> (j * 8);
+	}
+
+	slot->len = min_t(unsigned int, IBIR_XFER_BYTES(ibir),
+			  dev->ibi->max_payload_len);
+	i3c_master_queue_ibi(dev, slot);
+	data_consumed = true;
+
+out_unlock:
+	spin_unlock(&master->ibi.lock);
+
+out:
+	/* Consume data from the FIFO if it's not been done already. */
+	if (!data_consumed) {
+		for (i = 0; i < IBIR_XFER_BYTES(ibir); i += 4)
+			readl(master->regs + IBI_DATA_FIFO);
+	}
+}
+
+static void cnds_i3c_master_demux_ibis(struct cdns_i3c_master *master)
+{
+	u32 status0;
+
+	writel(MST_INT_IBIR_THR, master->regs + MST_ICR);
+
+	for (status0 = readl(master->regs + MST_STATUS0);
+	     !(status0 & MST_STATUS0_IBIR_EMP);
+	     status0 = readl(master->regs + MST_STATUS0)) {
+		u32 ibir = readl(master->regs + IBIR);
+
+		switch (IBIR_TYPE(ibir)) {
+		case IBIR_TYPE_IBI:
+			cdns_i3c_master_handle_ibi(master, ibir);
+			break;
+
+		case IBIR_TYPE_HJ:
+			WARN_ON(IBIR_XFER_BYTES(ibir) || (ibir & IBIR_ERROR));
+			queue_work(master->base.wq, &master->hj_work);
+			break;
+
+		case IBIR_TYPE_MR:
+			WARN_ON(IBIR_XFER_BYTES(ibir) || (ibir & IBIR_ERROR));
+		default:
+			break;
+		}
+	}
+}
+
+static irqreturn_t cdns_i3c_master_interrupt(int irq, void *data)
+{
+	struct cdns_i3c_master *master = data;
+	u32 status;
+
+	status = readl(master->regs + MST_ISR);
+	if (!(status & readl(master->regs + MST_IMR)))
+		return IRQ_NONE;
+
+	spin_lock(&master->xferqueue.lock);
+	cdns_i3c_master_end_xfer_locked(master, status);
+	spin_unlock(&master->xferqueue.lock);
+
+	if (status & MST_INT_IBIR_THR)
+		cnds_i3c_master_demux_ibis(master);
+
+	return IRQ_HANDLED;
+}
+
+static int cdns_i3c_master_disable_ibi(struct i3c_device *dev)
+{
+	struct i3c_master_controller *m = i3c_device_get_master(dev);
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	struct cdns_i3c_i2c_dev_data *data = i3c_device_get_master_data(dev);
+	unsigned long flags;
+	u32 sirmap;
+	int ret;
+
+	ret = i3c_master_disec_locked(m, dev->info.dyn_addr,
+				      I3C_CCC_EVENT_SIR);
+	if (ret)
+		return ret;
+
+	spin_lock_irqsave(&master->ibi.lock, flags);
+	sirmap = readl(master->regs + SIR_MAP_DEV_REG(data->ibi));
+	sirmap &= ~SIR_MAP_DEV_CONF_MASK(data->ibi);
+	sirmap |= SIR_MAP_DEV_CONF(data->ibi,
+				   SIR_MAP_DEV_DA(I3C_BROADCAST_ADDR));
+	writel(sirmap, master->regs + SIR_MAP_DEV_REG(data->ibi));
+	spin_unlock_irqrestore(&master->ibi.lock, flags);
+
+	return ret;
+}
+
+static int cdns_i3c_master_enable_ibi(struct i3c_device *dev)
+{
+	struct i3c_master_controller *m = i3c_device_get_master(dev);
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	struct cdns_i3c_i2c_dev_data *data = i3c_device_get_master_data(dev);
+	unsigned long flags;
+	u32 sircfg, sirmap;
+	int ret;
+
+	spin_lock_irqsave(&master->ibi.lock, flags);
+	sirmap = readl(master->regs + SIR_MAP_DEV_REG(data->ibi));
+	sirmap &= ~SIR_MAP_DEV_CONF_MASK(data->ibi);
+	sircfg = SIR_MAP_DEV_ROLE(dev->info.bcr >> 6) |
+		 SIR_MAP_DEV_DA(dev->info.dyn_addr) |
+		 SIR_MAP_DEV_PL(dev->info.max_ibi_len) |
+		 SIR_MAP_DEV_ACK;
+
+	if (dev->info.bcr & I3C_BCR_MAX_DATA_SPEED_LIM)
+		sircfg |= SIR_MAP_DEV_SLOW;
+
+	sirmap |= SIR_MAP_DEV_CONF(data->ibi, sircfg);
+	writel(sirmap, master->regs + SIR_MAP_DEV_REG(data->ibi));
+	spin_unlock_irqrestore(&master->ibi.lock, flags);
+
+	ret = i3c_master_enec_locked(m, dev->info.dyn_addr,
+				     I3C_CCC_EVENT_SIR);
+	if (ret) {
+		spin_lock_irqsave(&master->ibi.lock, flags);
+		sirmap = readl(master->regs + SIR_MAP_DEV_REG(data->ibi));
+		sirmap &= ~SIR_MAP_DEV_CONF_MASK(data->ibi);
+		sirmap |= SIR_MAP_DEV_CONF(data->ibi,
+					   SIR_MAP_DEV_DA(I3C_BROADCAST_ADDR));
+		writel(sirmap, master->regs + SIR_MAP_DEV_REG(data->ibi));
+		spin_unlock_irqrestore(&master->ibi.lock, flags);
+	}
+
+	return ret;
+}
+
+static int cdns_i3c_master_request_ibi(struct i3c_device *dev,
+				       const struct i3c_ibi_setup *req)
+{
+	struct i3c_master_controller *m = i3c_device_get_master(dev);
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	struct cdns_i3c_i2c_dev_data *data = i3c_device_get_master_data(dev);
+	unsigned long flags;
+	unsigned int i;
+
+	data->ibi_pool = i3c_generic_ibi_alloc_pool(dev, req);
+	if (IS_ERR(data->ibi_pool))
+		return PTR_ERR(data->ibi_pool);
+
+	spin_lock_irqsave(&master->ibi.lock, flags);
+	for (i = 0; i < master->ibi.num_slots; i++) {
+		if (!master->ibi.slots[i]) {
+			data->ibi = i;
+			master->ibi.slots[i] = dev;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&master->ibi.lock, flags);
+
+	if (i < master->ibi.num_slots)
+		return 0;
+
+	i3c_generic_ibi_free_pool(data->ibi_pool);
+	data->ibi_pool = NULL;
+
+	return -ENOSPC;
+}
+
+static void cdns_i3c_master_free_ibi(struct i3c_device *dev)
+{
+	struct i3c_master_controller *m = i3c_device_get_master(dev);
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	struct cdns_i3c_i2c_dev_data *data = i3c_device_get_master_data(dev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&master->ibi.lock, flags);
+	master->ibi.slots[data->ibi] = NULL;
+	data->ibi = -1;
+	spin_unlock_irqrestore(&master->ibi.lock, flags);
+
+	i3c_generic_ibi_free_pool(data->ibi_pool);
+}
+
+static void cdns_i3c_master_recycle_ibi_slot(struct i3c_device *dev,
+					     struct i3c_ibi_slot *slot)
+{
+	struct cdns_i3c_i2c_dev_data *data = i3c_device_get_master_data(dev);
+
+	i3c_generic_ibi_recycle_slot(data->ibi_pool, slot);
+}
+
+static const struct i3c_master_controller_ops cdns_i3c_master_ops = {
+	.bus_init = cdns_i3c_master_bus_init,
+	.bus_cleanup = cdns_i3c_master_bus_cleanup,
+	.do_daa = cdns_i3c_master_do_daa,
+	.attach_i3c_dev = cdns_i3c_master_attach_i3c_dev,
+	.reattach_i3c_dev = cdns_i3c_master_reattach_i3c_dev,
+	.detach_i3c_dev = cdns_i3c_master_detach_i3c_dev,
+	.attach_i2c_dev = cdns_i3c_master_attach_i2c_dev,
+	.detach_i2c_dev = cdns_i3c_master_detach_i2c_dev,
+	.supports_ccc_cmd = cdns_i3c_master_supports_ccc_cmd,
+	.send_ccc_cmd = cdns_i3c_master_send_ccc_cmd,
+	.priv_xfers = cdns_i3c_master_priv_xfers,
+	.i2c_xfers = cdns_i3c_master_i2c_xfers,
+	.i2c_funcs = cdns_i3c_master_i2c_funcs,
+	.enable_ibi = cdns_i3c_master_enable_ibi,
+	.disable_ibi = cdns_i3c_master_disable_ibi,
+	.request_ibi = cdns_i3c_master_request_ibi,
+	.free_ibi = cdns_i3c_master_free_ibi,
+	.recycle_ibi_slot = cdns_i3c_master_recycle_ibi_slot,
+};
+
+static void cdns_i3c_master_hj(struct work_struct *work)
+{
+	struct cdns_i3c_master *master = container_of(work,
+						      struct cdns_i3c_master,
+						      hj_work);
+
+	i3c_master_do_daa(&master->base);
+}
+
+static int cdns_i3c_master_probe(struct platform_device *pdev)
+{
+	struct cdns_i3c_master *master;
+	struct resource *res;
+	int ret, irq;
+	u32 val;
+
+	master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
+	if (!master)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	master->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(master->regs))
+		return PTR_ERR(master->regs);
+
+	master->pclk = devm_clk_get(&pdev->dev, "pclk");
+	if (IS_ERR(master->pclk))
+		return PTR_ERR(master->pclk);
+
+	master->sysclk = devm_clk_get(&pdev->dev, "sysclk");
+	if (IS_ERR(master->pclk))
+		return PTR_ERR(master->pclk);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = clk_prepare_enable(master->pclk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(master->sysclk);
+	if (ret)
+		goto err_disable_pclk;
+
+	if (readl(master->regs + DEV_ID) != DEV_ID_I3C_MASTER) {
+		ret = -EINVAL;
+		goto err_disable_sysclk;
+	}
+
+	spin_lock_init(&master->xferqueue.lock);
+	INIT_LIST_HEAD(&master->xferqueue.list);
+
+	INIT_WORK(&master->hj_work, cdns_i3c_master_hj);
+	writel(0xffffffff, master->regs + MST_IDR);
+	writel(0xffffffff, master->regs + SLV_IDR);
+	ret = devm_request_irq(&pdev->dev, irq, cdns_i3c_master_interrupt, 0,
+			       dev_name(&pdev->dev), master);
+	if (ret)
+		goto err_disable_sysclk;
+
+	platform_set_drvdata(pdev, master);
+
+	val = readl(master->regs + CONF_STATUS0);
+
+	/* Device ID0 is reserved to describe this master. */
+	master->maxdevs = CONF_STATUS0_DEVS_NUM(val);
+	master->free_rr_slots = GENMASK(master->maxdevs, 1);
+
+	val = readl(master->regs + CONF_STATUS1);
+	master->caps.cmdfifodepth = CONF_STATUS1_CMD_DEPTH(val);
+	master->caps.rxfifodepth = CONF_STATUS1_RX_DEPTH(val);
+	master->caps.txfifodepth = CONF_STATUS1_TX_DEPTH(val);
+	master->caps.ibirfifodepth = CONF_STATUS0_IBIR_DEPTH(val);
+	master->caps.cmdrfifodepth = CONF_STATUS0_CMDR_DEPTH(val);
+
+	spin_lock_init(&master->ibi.lock);
+	master->ibi.num_slots = CONF_STATUS1_IBI_HW_RES(val);
+	master->ibi.slots = devm_kzalloc(&pdev->dev,
+					 sizeof(*master->ibi.slots) *
+					 master->ibi.num_slots,
+					 GFP_KERNEL);
+	if (!master->ibi.slots)
+		goto err_disable_sysclk;
+
+	writel(IBIR_THR(1), master->regs + CMD_IBI_THR_CTRL);
+	writel(MST_INT_IBIR_THR, master->regs + MST_IER);
+	writel(DEVS_CTRL_DEV_CLR_ALL, master->regs + DEVS_CTRL);
+
+	ret = i3c_master_register(&master->base, &pdev->dev,
+				  &cdns_i3c_master_ops, false);
+	if (ret)
+		goto err_disable_sysclk;
+
+	return 0;
+
+err_disable_sysclk:
+	clk_disable_unprepare(master->sysclk);
+
+err_disable_pclk:
+	clk_disable_unprepare(master->pclk);
+
+	return ret;
+}
+
+static int cdns_i3c_master_remove(struct platform_device *pdev)
+{
+	struct cdns_i3c_master *master = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = i3c_master_unregister(&master->base);
+	if (ret)
+		return ret;
+
+	clk_disable_unprepare(master->sysclk);
+	clk_disable_unprepare(master->pclk);
+
+	return 0;
+}
+
+static const struct of_device_id cdns_i3c_master_of_ids[] = {
+	{ .compatible = "cdns,i3c-master" },
+	{ /* sentinel */ },
+};
+
+static struct platform_driver cdns_i3c_master = {
+	.probe = cdns_i3c_master_probe,
+	.remove = cdns_i3c_master_remove,
+	.driver = {
+		.name = "cdns-i3c-master",
+		.of_match_table = cdns_i3c_master_of_ids,
+	},
+};
+module_platform_driver(cdns_i3c_master);
+
+MODULE_AUTHOR("Boris Brezillon <boris.brezillon@bootlin.com>");
+MODULE_DESCRIPTION("Cadence I3C master driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:cdns-i3c-master");
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 08/10] dt-bindings: i3c: Document Cadence I3C master bindings

[Boris Brezillon]

Document Cadence I3C master DT bindings.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v5:
- Add Rob's R-b

Changes in v4:
- Fix example to match the new representation
---
 .../devicetree/bindings/i3c/cdns,i3c-master.txt    | 44 ++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt

diff --git a/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt b/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt
new file mode 100644
index 000000000000..0e2b8b8770bf
--- /dev/null
+++ b/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt
@@ -0,0 +1,44 @@
+Bindings for cadence I3C master block
+=====================================
+
+Required properties:
+--------------------
+- compatible: shall be "cdns,i3c-master"
+- clocks: shall reference the pclk and sysclk
+- clock-names: shall contain "pclk" and "sysclk"
+- interrupts: the interrupt line connected to this I3C master
+- reg: I3C master registers
+
+Mandatory properties defined by the generic binding (see
+Documentation/devicetree/bindings/i3c/i3c.txt for more details):
+
+- #address-cells: shall be set to 1
+- #size-cells: shall be set to 0
+
+Optional properties defined by the generic binding (see
+Documentation/devicetree/bindings/i3c/i3c.txt for more details):
+
+- i2c-scl-hz
+- i3c-scl-hz
+
+I3C device connected on the bus follow the generic description (see
+Documentation/devicetree/bindings/i3c/i3c.txt for more details).
+
+Example:
+
+	i3c-master@0d040000 {
+		compatible = "cdns,i3c-master";
+		clocks = <&coreclock>, <&i3csysclock>;
+		clock-names = "pclk", "sysclk";
+		interrupts = <3 0>;
+		reg = <0x0d040000 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		i2c-scl-hz = <100000>;
+
+		nunchuk: nunchuk@52 {
+			compatible = "nintendo,nunchuk";
+			reg = <0x52 0x80000010 0>;
+		};
+	};
+
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 09/10] gpio: Add a driver for Cadence I3C GPIO expander

[Boris Brezillon]

Add a driver for Cadence I3C GPIO expander.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Hi Linus,

I intentionally did not report your Acked-by because of the scract
buffer changes I added in this version (needed to follow the
"buffer should be DMA-able" rule)

Feel free to add it back if you're okay with this version.

Regards,

Boris

Changes in v5:
- Use the !! operator to return 0 or 1 in cdns_i3c_gpio_get_direction()
- Use a scratch buffer to make sure the buffers passed to
  i3c_device_do_priv_xfers() are DMA-able
- Fix errors reported by checkpatch

Changes in v4:
- none

Changes in v3:
- new
---
 drivers/gpio/Kconfig         |  11 ++
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-cdns-i3c.c | 411 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 423 insertions(+)
 create mode 100644 drivers/gpio/gpio-cdns-i3c.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 71c0ab46f216..19ed6006aea1 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -898,6 +898,17 @@ config GPIO_TS4900
 
 endmenu
 
+menu "I3C GPIO expanders"
+	depends on I3C
+
+config GPIO_CDNS_I3C
+	tristate "Cadence I3C GPIO expander"
+	select GPIOLIB_IRQCHIP
+	help
+	  Say yes here to enabled the driver for Cadence I3C GPIO expander.
+
+endmenu
+
 menu "MFD GPIO expanders"
 
 config GPIO_ADP5520
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 1324c8f966a7..020b9171223b 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_GPIO_BCM_KONA)	+= gpio-bcm-kona.o
 obj-$(CONFIG_GPIO_BD9571MWV)	+= gpio-bd9571mwv.o
 obj-$(CONFIG_GPIO_BRCMSTB)	+= gpio-brcmstb.o
 obj-$(CONFIG_GPIO_BT8XX)	+= gpio-bt8xx.o
+obj-$(CONFIG_GPIO_CDNS_I3C)	+= gpio-cdns-i3c.o
 obj-$(CONFIG_GPIO_CLPS711X)	+= gpio-clps711x.o
 obj-$(CONFIG_GPIO_CS5535)	+= gpio-cs5535.o
 obj-$(CONFIG_GPIO_CRYSTAL_COVE)	+= gpio-crystalcove.o
diff --git a/drivers/gpio/gpio-cdns-i3c.c b/drivers/gpio/gpio-cdns-i3c.c
new file mode 100644
index 000000000000..028336754215
--- /dev/null
+++ b/drivers/gpio/gpio-cdns-i3c.c
@@ -0,0 +1,411 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Cadence Design Systems Inc.
+ *
+ * Author: Boris Brezillon <boris.brezillon@bootlin.com>
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/i3c/device.h>
+#include <linux/module.h>
+
+#define OVR		0x0
+#define IVR		0x1
+#define DIR_MODE	0x2
+#define IMR		0x3
+#define ISR		0x4
+#define ITR(x)		(0x5 + (x))
+
+struct cdns_i3c_gpio {
+	struct gpio_chip gpioc;
+	struct irq_chip irqc;
+	struct i3c_device *i3cdev;
+	struct mutex irq_lock;
+	u8 dir;
+	u8 ovr;
+	u8 imr;
+	u8 itr[3];
+};
+
+static struct cdns_i3c_gpio *gpioc_to_cdns_gpioc(struct gpio_chip *gpioc)
+{
+	return container_of(gpioc, struct cdns_i3c_gpio, gpioc);
+}
+
+static int cdns_i3c_gpio_read_reg(struct cdns_i3c_gpio *gpioc, u8 reg,
+				  u8 *val)
+{
+	struct i3c_priv_xfer xfers[2] = { };
+	u8 *scratchbuf;
+	int ret;
+
+	/*
+	 * i3c_device_do_priv_xfers() mandates that buffers passed in xfers be
+	 * DMA-able. This prevents us from using reg and val directly since
+	 * reg is on the stack, and val might be too.
+	 * Allocate a temporary buffer with kmalloc() to solve the problem.
+	 */
+	scratchbuf = kmalloc(sizeof(*scratchbuf), GFP_KERNEL);
+	if (!scratchbuf)
+		return -ENOMEM;
+
+	scratchbuf[0] = reg;
+	xfers[0].data.out = scratchbuf;
+	xfers[0].len = 1;
+	xfers[1].data.in = scratchbuf;
+	xfers[1].len = 1;
+	xfers[1].rnw = true;
+
+	ret = i3c_device_do_priv_xfers(gpioc->i3cdev, xfers,
+				       ARRAY_SIZE(xfers));
+	if (!ret)
+		*val = *scratchbuf;
+
+	kfree(scratchbuf);
+
+	return ret;
+}
+
+static int cdns_i3c_gpio_write_reg(struct cdns_i3c_gpio *gpioc, u8 reg,
+				   u8 val)
+{
+	struct i3c_priv_xfer xfers[2] = { };
+	u8 *scratchbuf;
+	int ret;
+
+	/*
+	 * i3c_device_do_priv_xfers() mandates that buffers passed in xfers be
+	 * DMA-able. This prevents us from using reg and val directly since
+	 * reg is on the stack, and val might be too.
+	 * Allocate a temporary buffer with kmalloc() to solve the problem.
+	 */
+	scratchbuf = kmalloc(sizeof(*scratchbuf) * 2, GFP_KERNEL);
+	if (!scratchbuf)
+		return -ENOMEM;
+
+	scratchbuf[0] = reg;
+	scratchbuf[1] = val;
+	xfers[0].data.out = scratchbuf;
+	xfers[0].len = 1;
+	xfers[1].data.out = scratchbuf + 1;
+	xfers[1].len = 1;
+
+	ret = i3c_device_do_priv_xfers(gpioc->i3cdev, xfers,
+				       ARRAY_SIZE(xfers));
+
+	kfree(scratchbuf);
+
+	return ret;
+}
+
+static int cdns_i3c_gpio_get_direction(struct gpio_chip *g,
+				       unsigned int offset)
+{
+	struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g);
+
+	return !!(gpioc->dir & BIT(offset));
+}
+
+static void cdns_i3c_gpio_set_multiple(struct gpio_chip *g,
+				       unsigned long *mask,
+				       unsigned long *bits)
+{
+	struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g);
+	u8 newovr;
+	int ret;
+
+	newovr = (gpioc->ovr & ~(*mask)) | (*bits & *mask);
+	if (newovr == gpioc->ovr)
+		return;
+
+	ret = cdns_i3c_gpio_write_reg(gpioc, OVR, newovr);
+	if (!ret)
+		gpioc->ovr = newovr;
+}
+
+static void cdns_i3c_gpio_set(struct gpio_chip *g, unsigned int offset,
+			      int value)
+{
+	unsigned long mask = BIT(offset), bits = value ? BIT(offset) : 0;
+
+	cdns_i3c_gpio_set_multiple(g, &mask, &bits);
+}
+
+static int cdns_i3c_gpio_set_dir(struct cdns_i3c_gpio *gpioc, unsigned int pin,
+				 bool in)
+{
+	u8 newdir;
+	int ret;
+
+	newdir = gpioc->dir;
+	if (in)
+		newdir |= BIT(pin);
+	else
+		newdir &= ~BIT(pin);
+
+	if (newdir == gpioc->dir)
+		return 0;
+
+	gpioc->dir = newdir;
+	ret = cdns_i3c_gpio_write_reg(gpioc, DIR_MODE, newdir);
+	if (!ret)
+		gpioc->dir = newdir;
+
+	return ret;
+}
+
+static int cdns_i3c_gpio_dir_input(struct gpio_chip *g, unsigned int offset)
+{
+	struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g);
+
+	return cdns_i3c_gpio_set_dir(gpioc, offset, true);
+}
+
+static int cdns_i3c_gpio_dir_output(struct gpio_chip *g, unsigned int offset,
+				    int val)
+{
+	struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g);
+
+	cdns_i3c_gpio_set(g, offset, val);
+
+	return cdns_i3c_gpio_set_dir(gpioc, offset, true);
+}
+
+static int cdns_i3c_gpio_get_multiple(struct gpio_chip *g,
+				      unsigned long *mask,
+				      unsigned long *bits)
+{
+	struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g);
+	int ret;
+	u8 ivr;
+
+	ret = cdns_i3c_gpio_read_reg(gpioc, IVR, &ivr);
+	if (ret)
+		return ret;
+
+	*bits = ivr & *mask & gpioc->dir;
+	*bits |= gpioc->ovr & *mask & ~gpioc->dir;
+
+	return 0;
+}
+
+static int cdns_i3c_gpio_get(struct gpio_chip *g, unsigned int offset)
+{
+	unsigned long mask = BIT(offset), bits = 0;
+	int ret;
+
+	ret = cdns_i3c_gpio_get_multiple(g, &mask, &bits);
+	if (ret)
+		return ret;
+
+	return mask & bits;
+}
+
+static void cdns_i3c_gpio_ibi_handler(struct i3c_device *i3cdev,
+				      const struct i3c_ibi_payload *payload)
+{
+	struct cdns_i3c_gpio *gpioc = i3cdev_get_drvdata(i3cdev);
+	u8 isr = 0;
+	int i;
+
+	cdns_i3c_gpio_read_reg(gpioc, ISR, &isr);
+	for (i = 0; i < 8; i++) {
+		unsigned int irq;
+
+		if (!(BIT(i) & isr & gpioc->imr))
+			continue;
+
+		irq = irq_find_mapping(gpioc->gpioc.irq.domain, i);
+		handle_nested_irq(irq);
+	}
+}
+
+static void cdns_i3c_gpio_irq_lock(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct cdns_i3c_gpio *gpioc = gpiochip_get_data(gc);
+
+	mutex_lock(&gpioc->irq_lock);
+}
+
+static void cdns_i3c_gpio_irq_sync_unlock(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct cdns_i3c_gpio *gpioc = gpiochip_get_data(gc);
+	int i;
+
+	cdns_i3c_gpio_write_reg(gpioc, IMR, gpioc->imr);
+	for (i = 0; i < 3; i++)
+		cdns_i3c_gpio_write_reg(gpioc, ITR(i), gpioc->itr[i]);
+
+	mutex_unlock(&gpioc->irq_lock);
+}
+
+static void cdns_i3c_gpio_irq_unmask(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct cdns_i3c_gpio *gpioc = gpiochip_get_data(gc);
+
+	gpioc->imr |= BIT(data->hwirq);
+}
+
+static void cdns_i3c_gpio_irq_mask(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct cdns_i3c_gpio *gpioc = gpiochip_get_data(gc);
+
+	gpioc->imr &= ~BIT(data->hwirq);
+}
+
+static int cdns_i3c_gpio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct cdns_i3c_gpio *gpioc = gpiochip_get_data(gc);
+
+	switch (type) {
+	case IRQ_TYPE_LEVEL_HIGH:
+		gpioc->itr[0] |= BIT(data->hwirq);
+		gpioc->itr[1] |= BIT(data->hwirq);
+		break;
+
+	case IRQ_TYPE_LEVEL_LOW:
+		gpioc->itr[0] |= BIT(data->hwirq);
+		gpioc->itr[1] &= ~BIT(data->hwirq);
+		break;
+
+	case IRQ_TYPE_EDGE_BOTH:
+		gpioc->itr[0] &= ~BIT(data->hwirq);
+		gpioc->itr[2] |= BIT(data->hwirq);
+		break;
+
+	case IRQ_TYPE_EDGE_RISING:
+		gpioc->itr[0] &= ~BIT(data->hwirq);
+		gpioc->itr[1] |= BIT(data->hwirq);
+		gpioc->itr[2] &= ~BIT(data->hwirq);
+		break;
+
+	case IRQ_TYPE_EDGE_FALLING:
+		gpioc->itr[0] &= ~BIT(data->hwirq);
+		gpioc->itr[1] &= ~BIT(data->hwirq);
+		gpioc->itr[2] &= ~BIT(data->hwirq);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int cdns_i3c_gpio_probe(struct i3c_device *i3cdev)
+{
+	struct cdns_i3c_gpio *gpioc;
+	struct device *parent = i3cdev_to_dev(i3cdev);
+	struct i3c_ibi_setup ibisetup = {
+		.max_payload_len = 2,
+		.num_slots = 1,
+		.handler = cdns_i3c_gpio_ibi_handler,
+	};
+	int ret;
+
+	gpioc = devm_kzalloc(parent, sizeof(*gpioc), GFP_KERNEL);
+	if (!gpioc)
+		return -ENOMEM;
+
+	gpioc->i3cdev = i3cdev;
+	i3cdev_set_drvdata(i3cdev, gpioc);
+
+	/* Mask all interrupts. */
+	ret = cdns_i3c_gpio_write_reg(gpioc, IMR, 0);
+	if (ret)
+		return ret;
+
+	/*
+	 * Clear the ISR after reading it, not when the IBI is is Acked by the
+	 * I3C master. This way we make sure we don't lose events.
+	 */
+	ret = cdns_i3c_gpio_write_reg(gpioc, ITR(3), 0xff);
+	if (ret)
+		return ret;
+
+	ret = cdns_i3c_gpio_read_reg(gpioc, DIR_MODE, &gpioc->dir);
+	if (ret)
+		return ret;
+
+	ret = cdns_i3c_gpio_read_reg(gpioc, OVR, &gpioc->ovr);
+	if (ret)
+		return ret;
+
+	ret = i3c_device_request_ibi(i3cdev, &ibisetup);
+	if (ret)
+		return ret;
+
+	gpioc->gpioc.label = dev_name(parent);
+	gpioc->gpioc.owner = THIS_MODULE;
+	gpioc->gpioc.parent = parent;
+	gpioc->gpioc.base = -1;
+	gpioc->gpioc.ngpio = 8;
+	gpioc->gpioc.can_sleep = true;
+	gpioc->gpioc.get_direction = cdns_i3c_gpio_get_direction;
+	gpioc->gpioc.direction_input = cdns_i3c_gpio_dir_input;
+	gpioc->gpioc.direction_output = cdns_i3c_gpio_dir_output;
+	gpioc->gpioc.get = cdns_i3c_gpio_get;
+	gpioc->gpioc.get_multiple = cdns_i3c_gpio_get_multiple;
+	gpioc->gpioc.set = cdns_i3c_gpio_set;
+	gpioc->gpioc.set_multiple = cdns_i3c_gpio_set_multiple;
+
+	ret = devm_gpiochip_add_data(parent, &gpioc->gpioc, gpioc);
+	if (ret)
+		return ret;
+
+	gpioc->irqc.name = dev_name(parent);
+	gpioc->irqc.parent_device = parent;
+	gpioc->irqc.irq_unmask = cdns_i3c_gpio_irq_unmask;
+	gpioc->irqc.irq_mask = cdns_i3c_gpio_irq_mask;
+	gpioc->irqc.irq_bus_lock = cdns_i3c_gpio_irq_lock;
+	gpioc->irqc.irq_bus_sync_unlock = cdns_i3c_gpio_irq_sync_unlock;
+	gpioc->irqc.irq_set_type = cdns_i3c_gpio_irq_set_type;
+	gpioc->irqc.flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_MASK_ON_SUSPEND;
+
+	ret = gpiochip_irqchip_add_nested(&gpioc->gpioc, &gpioc->irqc, 0,
+					  handle_simple_irq, IRQ_TYPE_NONE);
+	if (ret)
+		goto err_free_ibi;
+
+	ret = i3c_device_enable_ibi(i3cdev);
+	if (ret)
+		goto err_free_ibi;
+
+	return 0;
+
+err_free_ibi:
+	i3c_device_free_ibi(i3cdev);
+
+	return ret;
+}
+
+static int cdns_i3c_gpio_remove(struct i3c_device *i3cdev)
+{
+	i3c_device_disable_ibi(i3cdev);
+	i3c_device_free_ibi(i3cdev);
+
+	return 0;
+}
+
+static const struct i3c_device_id cdns_i3c_gpio_ids[] = {
+	I3C_DEVICE(0x1c9, 0x0, NULL),
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i3c, cdns_i3c_gpio_ids);
+
+static struct i3c_driver cdns_i3c_gpio = {
+	.driver.name = "cdns-i3c-gpio",
+	.id_table = cdns_i3c_gpio_ids,
+	.probe = cdns_i3c_gpio_probe,
+	.remove = cdns_i3c_gpio_remove,
+};
+module_i3c_driver(cdns_i3c_gpio);
+
+MODULE_AUTHOR("Boris Brezillon <boris.brezillon@bootlin.com>");
+MODULE_DESCRIPTION("Driver for Cadence I3C GPIO expander");
+MODULE_LICENSE("GPL v2");
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 10/10] dt-bindings: gpio: Add bindings for Cadence I3C gpio expander

[Boris Brezillon]

Document the Cadence I3C gpio expander bindings.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v5:
- Add Rob's R-b

Changes in v4:
- Use GPIO_ and IRQ_TYPE_ macros instead of raw numbers
- Fix the unit-address in the example
---
 .../devicetree/bindings/gpio/gpio-cdns-i3c.txt     | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-cdns-i3c.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-cdns-i3c.txt b/Documentation/devicetree/bindings/gpio/gpio-cdns-i3c.txt
new file mode 100644
index 000000000000..d0155a9cea79
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-cdns-i3c.txt
@@ -0,0 +1,39 @@
+* Cadence I3C GPIO expander
+
+The Cadence I3C GPIO expander provides 8 GPIOs controllable over I3C.
+This GPIOs can be configured in output or input mode and if they are in input
+mode they can generate IBIs (In Band Interrupts).
+
+Required properties for GPIO node:
+- reg : 3 cells encoding the I3C static address (none in our case) and the I3C
+	Provisional ID. See Documentation/devicetree/bindings/i3c/i3c.txt for
+	more details.
+	Should be <0x0 0x392 0x0>.
+- gpio-controller : Marks the device node as a gpio controller.
+- #gpio-cells : Should be two. The first cell is the pin number and
+  the second cell is used to specify the gpio polarity (GPIO_ACTIVE_HIGH or
+  GPIO_ACTIVE_LOW)
+- interrupt-controller: Marks the device node as an interrupt controller.
+- #interrupt-cells : Should be 2. The first cell is the GPIO number.
+  The second cell is used to specify trigger type and level flags.
+  The following trigger types are accepted (see
+  <dt-bindings/interrupt-controller/irq.h> for their definition):
+	IRQ_TYPE_EDGE_RISING
+	IRQ_TYPE_EDGE_FALLING
+	IRQ_TYPE_EDGE_BOTH
+	IRQ_TYPE_LEVEL_HIGH
+	IRQ_TYPE_LEVEL_LOW
+
+Example:
+
+	i3c-master@xxx {
+		...
+		i3c_gpio_expander: gpio@0,39200000000 {
+			reg = <0 0x392 0x0>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+		...
+	};
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 09/10] gpio: Add a driver for Cadence I3C GPIO expander

[Randy Dunlap]

Hi,

On 06/22/2018 03:49 AM, Boris Brezillon wrote:
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 71c0ab46f216..19ed6006aea1 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -898,6 +898,17 @@ config GPIO_TS4900
>  
>  endmenu
>  
> +menu "I3C GPIO expanders"
> +	depends on I3C
> +
> +config GPIO_CDNS_I3C
> +	tristate "Cadence I3C GPIO expander"
> +	select GPIOLIB_IRQCHIP
> +	help
> +	  Say yes here to enabled the driver for Cadence I3C GPIO expander.

	               to enable the driver

> +
> +endmenu


-- 
~Randy
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 09/10] gpio: Add a driver for Cadence I3C GPIO expander

[Boris Brezillon]

On Fri, 22 Jun 2018 09:04:47 -0700
Randy Dunlap <rdunlap@infradead.org> wrote:

> Hi,
> 
> On 06/22/2018 03:49 AM, Boris Brezillon wrote:
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 71c0ab46f216..19ed6006aea1 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -898,6 +898,17 @@ config GPIO_TS4900
> >  
> >  endmenu
> >  
> > +menu "I3C GPIO expanders"
> > +	depends on I3C
> > +
> > +config GPIO_CDNS_I3C
> > +	tristate "Cadence I3C GPIO expander"
> > +	select GPIOLIB_IRQCHIP
> > +	help
> > +	  Say yes here to enabled the driver for Cadence I3C GPIO expander.  
> 
> 	               to enable the driver

I'll fix this typo

Thanks,

Boris

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 01/10] i3c: Add core I3C infrastructure

[Peter Rosin]

On 2018-06-22 12:49, 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.

Are bus multiplexers relevant to I3C? The locking needed for handling
muxes for I2C is, well, convoluted...

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 01/10] i3c: Add core I3C infrastructure

[Boris Brezillon]

Hi Peter,

On Fri, 22 Jun 2018 23:35:34 +0200
Peter Rosin <peda@axentia.se> wrote:

> On 2018-06-22 12:49, 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.  
> 
> Are bus multiplexers relevant to I3C?

Not yet, but who knows.

> The locking needed for handling
> muxes for I2C is, well, convoluted...

Do you remember what was the problem?

Anyway, I'd really like to have basic support upstreamed before we
start considering advanced use cases that do not exist yet. Don't get
me wrong, I'm not against having the multiplexer/locking discussion,
but it should not block inclusion of the I3C subsystem.

Regards,

Boris
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 01/10] i3c: Add core I3C infrastructure

[Peter Rosin]

On 2018-06-23 12:17, Boris Brezillon wrote:
> Hi Peter,
> 
> On Fri, 22 Jun 2018 23:35:34 +0200
> Peter Rosin <peda@axentia.se> wrote:
> 
>> On 2018-06-22 12:49, 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.  
>>
>> Are bus multiplexers relevant to I3C?
> 
> Not yet, but who knows.
> 
>> The locking needed for handling
>> muxes for I2C is, well, convoluted...
> 
> Do you remember what was the problem?
> 
> Anyway, I'd really like to have basic support upstreamed before we
> start considering advanced use cases that do not exist yet. Don't get
> me wrong, I'm not against having the multiplexer/locking discussion,
> but it should not block inclusion of the I3C subsystem.

I'm trying to avoid the unfortunate situation in I2C where there
are two slightly incompatible locking schemes for muxes. There's
probably nothing to worry about until the first I3C mux is added
though. But since I2C devices are supposedly compatible with I3C
that might be the case from day one?

---

If I read your code right, I3C client drivers will typically call
i3c_device_do_priv_xfer (instead of i2c_transfer/i2c_smbus_xfer)
and i3c_device_do_prov_xfer will grab the i3c_bus_normaluse_lock
during the transfer. This seems equivalent to normal use in
I2C with i2c_transfer/i2c_smbus_xfer.

When muxes are thrown into the mix, you find yourself needing to
do more than the "real" transfer with some lock held. In I2C there
is an unlocked __i2c_transfer, and locking/unlocking is exposed.
Muxes typically grab the lock, set the mux in the appropriate
position, do an unlocked __i2c_transfer, optionally sets the mux
in some default position and then lets go of the lock. See e.g.
i2c/muxes/i2c-mux-pca954x.c

However, that is the simple case. There are also muxes that are
controlled with GPIO pins, and that gets hairy if the GPIO pins
are controlled from the same I2C bus that is muxed. The GPIO
driver would have to know that some GPIO pins need to use unlocked
I2C transfers for that to work with the above locking scheme. And
no GPIO driver does not want to know about that at all. I.e. you
have two fundamentally different requirement depending on if the
GPIO pins controlling the mux are controlled using the muxed bus
or if the pins are controlled in some completely unrelated way.
The latter case is probably the normal case, with the GPIO mux
controlled directly from some SoC pins. In the latter case you
also want to prevent any transfer on the bus while the GPIO pins
for the mux are changing, i.e. the total opposite of the former
case. It gets really really hairy if you have multiple levels
of muxes...

There are a some old drivers (e.g. i2c/busses/i2c-amd756-s4882.c)
that handles this by simply bypassing the GPIO subsystem, but
that is not really an option if some pins are used to mux the
I2C bus while some pins are used for other things.

I don't know if this affects I3C before muxes are added, but I
suspect muxes will happen sooner rather than later, since the
spec mentions that the bus only support 11 devices maximum. 11
don't seem like a lot, and it seems likely that there will be
a need to have more devices somehow...

But just maybe, in order to not run into the above situation, it
needs to be handled right from the start with preparatory and
cleanup stages of each transfers, or something?

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 01/10] i3c: Add core I3C infrastructure

[Boris Brezillon]

On Sat, 23 Jun 2018 23:40:36 +0200
Peter Rosin <peda@axentia.se> wrote:

> On 2018-06-23 12:17, Boris Brezillon wrote:
> > Hi Peter,
> > 
> > On Fri, 22 Jun 2018 23:35:34 +0200
> > Peter Rosin <peda@axentia.se> wrote:
> >   
> >> On 2018-06-22 12:49, 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.    
> >>
> >> Are bus multiplexers relevant to I3C?  
> > 
> > Not yet, but who knows.
> >   
> >> The locking needed for handling
> >> muxes for I2C is, well, convoluted...  
> > 
> > Do you remember what was the problem?
> > 
> > Anyway, I'd really like to have basic support upstreamed before we
> > start considering advanced use cases that do not exist yet. Don't get
> > me wrong, I'm not against having the multiplexer/locking discussion,
> > but it should not block inclusion of the I3C subsystem.  
> 
> I'm trying to avoid the unfortunate situation in I2C where there
> are two slightly incompatible locking schemes for muxes. There's
> probably nothing to worry about until the first I3C mux is added
> though. But since I2C devices are supposedly compatible with I3C
> that might be the case from day one?

The I²C backward compatibility is implemented in a pretty simple way, so
I don't think you'll have problems coming from the I3C part on this
(unless it needs special hooks in i2c_adapter to work properly). This
being said, if the I2C framework is already not able to properly
handle the cases you describe below, the I3C layer won't solve
that :-P. 

> 
> ---
> 
> If I read your code right, I3C client drivers will typically call
> i3c_device_do_priv_xfer (instead of i2c_transfer/i2c_smbus_xfer)
> and i3c_device_do_prov_xfer will grab the i3c_bus_normaluse_lock
> during the transfer. This seems equivalent to normal use in
> I2C with i2c_transfer/i2c_smbus_xfer.

Note that the bus lock is a read/write lock which is mostly taken in
read mode (AKA normaluse mode). The only situation where it's taken in
write mode (AKA maintenance mode) is when a bus maintenance operation is
done. In this case, we need to block all future transfers, because
maintenance operations might change dynamic device addresses, which
would make future transfers irrelevant if they were queued before the
maintenance operation is finished. 

The bus lock does not guarantee proper serialization of bus accesses.
This task is left to the controller drivers, since this is what they
tend to optimize (by queuing transfers at the HW level).

> 
> When muxes are thrown into the mix, you find yourself needing to
> do more than the "real" transfer with some lock held. In I2C there
> is an unlocked __i2c_transfer, and locking/unlocking is exposed.
> Muxes typically grab the lock, set the mux in the appropriate
> position, do an unlocked __i2c_transfer, optionally sets the mux
> in some default position and then lets go of the lock. See e.g.
> i2c/muxes/i2c-mux-pca954x.c
> 
> However, that is the simple case. There are also muxes that are
> controlled with GPIO pins, and that gets hairy if the GPIO pins
> are controlled from the same I2C bus that is muxed. The GPIO
> driver would have to know that some GPIO pins need to use unlocked
> I2C transfers for that to work with the above locking scheme. And
> no GPIO driver does not want to know about that at all. I.e. you
> have two fundamentally different requirement depending on if the
> GPIO pins controlling the mux are controlled using the muxed bus
> or if the pins are controlled in some completely unrelated way.
> The latter case is probably the normal case, with the GPIO mux
> controlled directly from some SoC pins. In the latter case you
> also want to prevent any transfer on the bus while the GPIO pins
> for the mux are changing, i.e. the total opposite of the former
> case. It gets really really hairy if you have multiple levels
> of muxes...
> 
> There are a some old drivers (e.g. i2c/busses/i2c-amd756-s4882.c)
> that handles this by simply bypassing the GPIO subsystem, but
> that is not really an option if some pins are used to mux the
> I2C bus while some pins are used for other things.

I see.

> 
> I don't know if this affects I3C before muxes are added, but I
> suspect muxes will happen sooner rather than later, since the
> spec mentions that the bus only support 11 devices maximum. 11
> don't seem like a lot, and it seems likely that there will be
> a need to have more devices somehow...

I can't tell, and that's the whole problem here. How can you design a
solution for something that does not exist yet? Fixing the I2C muxing
logic, if it needs to be, is something I can understand. But how can you
envision what I3C muxes (if they ever exist) will look like?

> 
> But just maybe, in order to not run into the above situation, it
> needs to be handled right from the start with preparatory and
> cleanup stages of each transfers, or something?

How about applying this approach to I2C first and see how it flies.
Changing the I3C framework afterwards (when I3C muxes come in)
shouldn't be that complicated.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 01/10] i3c: Add core I3C infrastructure

[Peter Rosin]

On 2018-06-24 14:02, Boris Brezillon wrote:
> On Sat, 23 Jun 2018 23:40:36 +0200
> Peter Rosin <peda@axentia.se> wrote:
> 
>> On 2018-06-23 12:17, Boris Brezillon wrote:
>>> Hi Peter,
>>>
>>> On Fri, 22 Jun 2018 23:35:34 +0200
>>> Peter Rosin <peda@axentia.se> wrote:
>>>   
>>>> On 2018-06-22 12:49, 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.    
>>>>
>>>> Are bus multiplexers relevant to I3C?  
>>>
>>> Not yet, but who knows.
>>>   
>>>> The locking needed for handling
>>>> muxes for I2C is, well, convoluted...  
>>>
>>> Do you remember what was the problem?
>>>
>>> Anyway, I'd really like to have basic support upstreamed before we
>>> start considering advanced use cases that do not exist yet. Don't get
>>> me wrong, I'm not against having the multiplexer/locking discussion,
>>> but it should not block inclusion of the I3C subsystem.  
>>
>> I'm trying to avoid the unfortunate situation in I2C where there
>> are two slightly incompatible locking schemes for muxes. There's
>> probably nothing to worry about until the first I3C mux is added
>> though. But since I2C devices are supposedly compatible with I3C
>> that might be the case from day one?
> 
> The I²C backward compatibility is implemented in a pretty simple way, so
> I don't think you'll have problems coming from the I3C part on this
> (unless it needs special hooks in i2c_adapter to work properly). This
> being said, if the I2C framework is already not able to properly
> handle the cases you describe below, the I3C layer won't solve
> that :-P. 
> 
>>
>> ---
>>
>> If I read your code right, I3C client drivers will typically call
>> i3c_device_do_priv_xfer (instead of i2c_transfer/i2c_smbus_xfer)
>> and i3c_device_do_prov_xfer will grab the i3c_bus_normaluse_lock
>> during the transfer. This seems equivalent to normal use in
>> I2C with i2c_transfer/i2c_smbus_xfer.
> 
> Note that the bus lock is a read/write lock which is mostly taken in
> read mode (AKA normaluse mode). The only situation where it's taken in
> write mode (AKA maintenance mode) is when a bus maintenance operation is
> done. In this case, we need to block all future transfers, because
> maintenance operations might change dynamic device addresses, which
> would make future transfers irrelevant if they were queued before the
> maintenance operation is finished. 
> 
> The bus lock does not guarantee proper serialization of bus accesses.
> This task is left to the controller drivers, since this is what they
> tend to optimize (by queuing transfers at the HW level).

Oh. Will that design decision (localized serialization) not make it
extremely hard to implement muxing (and gating and other stuff that
you need, at least for I2C) that is controller independent?

>> When muxes are thrown into the mix, you find yourself needing to
>> do more than the "real" transfer with some lock held. In I2C there
>> is an unlocked __i2c_transfer, and locking/unlocking is exposed.
>> Muxes typically grab the lock, set the mux in the appropriate
>> position, do an unlocked __i2c_transfer, optionally sets the mux
>> in some default position and then lets go of the lock. See e.g.
>> i2c/muxes/i2c-mux-pca954x.c
>>
>> However, that is the simple case. There are also muxes that are
>> controlled with GPIO pins, and that gets hairy if the GPIO pins
>> are controlled from the same I2C bus that is muxed. The GPIO
>> driver would have to know that some GPIO pins need to use unlocked
>> I2C transfers for that to work with the above locking scheme. And
>> no GPIO driver does not want to know about that at all. I.e. you
>> have two fundamentally different requirement depending on if the
>> GPIO pins controlling the mux are controlled using the muxed bus
>> or if the pins are controlled in some completely unrelated way.
>> The latter case is probably the normal case, with the GPIO mux
>> controlled directly from some SoC pins. In the latter case you
>> also want to prevent any transfer on the bus while the GPIO pins
>> for the mux are changing, i.e. the total opposite of the former
>> case. It gets really really hairy if you have multiple levels
>> of muxes...
>>
>> There are a some old drivers (e.g. i2c/busses/i2c-amd756-s4882.c)
>> that handles this by simply bypassing the GPIO subsystem, but
>> that is not really an option if some pins are used to mux the
>> I2C bus while some pins are used for other things.
> 
> I see.
> 
>>
>> I don't know if this affects I3C before muxes are added, but I
>> suspect muxes will happen sooner rather than later, since the
>> spec mentions that the bus only support 11 devices maximum. 11
>> don't seem like a lot, and it seems likely that there will be
>> a need to have more devices somehow...
> 
> I can't tell, and that's the whole problem here. How can you design a
> solution for something that does not exist yet? Fixing the I2C muxing
> logic, if it needs to be, is something I can understand. But how can you
> envision what I3C muxes (if they ever exist) will look like?

Yeah, you have a point there. One problem is that I don't even see
how the situation can be unified for I2C...

>>
>> But just maybe, in order to not run into the above situation, it
>> needs to be handled right from the start with preparatory and
>> cleanup stages of each transfers, or something?
> 
> How about applying this approach to I2C first and see how it flies.
> Changing the I3C framework afterwards (when I3C muxes come in)
> shouldn't be that complicated.

That would require more thinking first, and I fear that the overhaul
will be bigger than what is called for given the rather fringe cases
that cause problems.

Note that I'm not trying to block I3C, I'm just trying to trigger
some thinking before the fact...

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 01/10] i3c: Add core I3C infrastructure

[Boris Brezillon]

On Sun, 24 Jun 2018 23:55:34 +0200
Peter Rosin <peda@axentia.se> wrote:

> On 2018-06-24 14:02, Boris Brezillon wrote:
> > On Sat, 23 Jun 2018 23:40:36 +0200
> > Peter Rosin <peda@axentia.se> wrote:
> >   
> >> On 2018-06-23 12:17, Boris Brezillon wrote:  
> >>> Hi Peter,
> >>>
> >>> On Fri, 22 Jun 2018 23:35:34 +0200
> >>> Peter Rosin <peda@axentia.se> wrote:
> >>>     
> >>>> On 2018-06-22 12:49, 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.      
> >>>>
> >>>> Are bus multiplexers relevant to I3C?    
> >>>
> >>> Not yet, but who knows.
> >>>     
> >>>> The locking needed for handling
> >>>> muxes for I2C is, well, convoluted...    
> >>>
> >>> Do you remember what was the problem?
> >>>
> >>> Anyway, I'd really like to have basic support upstreamed before we
> >>> start considering advanced use cases that do not exist yet. Don't get
> >>> me wrong, I'm not against having the multiplexer/locking discussion,
> >>> but it should not block inclusion of the I3C subsystem.    
> >>
> >> I'm trying to avoid the unfortunate situation in I2C where there
> >> are two slightly incompatible locking schemes for muxes. There's
> >> probably nothing to worry about until the first I3C mux is added
> >> though. But since I2C devices are supposedly compatible with I3C
> >> that might be the case from day one?  
> > 
> > The I²C backward compatibility is implemented in a pretty simple way, so
> > I don't think you'll have problems coming from the I3C part on this
> > (unless it needs special hooks in i2c_adapter to work properly). This
> > being said, if the I2C framework is already not able to properly
> > handle the cases you describe below, the I3C layer won't solve
> > that :-P. 
> >   
> >>
> >> ---
> >>
> >> If I read your code right, I3C client drivers will typically call
> >> i3c_device_do_priv_xfer (instead of i2c_transfer/i2c_smbus_xfer)
> >> and i3c_device_do_prov_xfer will grab the i3c_bus_normaluse_lock
> >> during the transfer. This seems equivalent to normal use in
> >> I2C with i2c_transfer/i2c_smbus_xfer.  
> > 
> > Note that the bus lock is a read/write lock which is mostly taken in
> > read mode (AKA normaluse mode). The only situation where it's taken in
> > write mode (AKA maintenance mode) is when a bus maintenance operation is
> > done. In this case, we need to block all future transfers, because
> > maintenance operations might change dynamic device addresses, which
> > would make future transfers irrelevant if they were queued before the
> > maintenance operation is finished. 
> > 
> > The bus lock does not guarantee proper serialization of bus accesses.
> > This task is left to the controller drivers, since this is what they
> > tend to optimize (by queuing transfers at the HW level).  
> 
> Oh. Will that design decision (localized serialization) not make it
> extremely hard to implement muxing (and gating and other stuff that
> you need, at least for I2C) that is controller independent?

The I²C framework has its own set of locks, and as I said earlier, I'm
just implementing an i2c_adapter, so every I²C transfer will go through
the standard I²C stack before being passed to the I3C framework (and
then the I3C controller driver). I3C transfers going on the bus should
have no impact here since they don't change the I2C mux state.

Regarding the fact that we might need a lock to get exclusive access on
the I3C bus, it might become true at some point, but it clearly isn't
right now. So instead of adding complexity for something we don't need,
I decided to only add the locking that I knew was required.

Anyway, we're making assumptions on things we're not able to test or
even validate based on a specification for a potential I3C mux, so I
really think we should wait for the problem to actually appear instead
of trying to fix it now.

Also, I have the feeling that I3C muxes will be closer to
routers/bridges in their approach (the master bus would encapsulate
frames that it wants to send on the sub-bus and the bridge would
extract those frames and forward them). Also, dummy muxing is a hard
thing to do in I3C because of the IBI stuff. If you really mux the bus
in HW, you'll lose the ability of receiving IBIs on the non-active
buses.
Of course, these are just speculations, but I don't see how dummy I3C
muxes could work.

> 
> >> When muxes are thrown into the mix, you find yourself needing to
> >> do more than the "real" transfer with some lock held. In I2C there
> >> is an unlocked __i2c_transfer, and locking/unlocking is exposed.
> >> Muxes typically grab the lock, set the mux in the appropriate
> >> position, do an unlocked __i2c_transfer, optionally sets the mux
> >> in some default position and then lets go of the lock. See e.g.
> >> i2c/muxes/i2c-mux-pca954x.c
> >>
> >> However, that is the simple case. There are also muxes that are
> >> controlled with GPIO pins, and that gets hairy if the GPIO pins
> >> are controlled from the same I2C bus that is muxed. The GPIO
> >> driver would have to know that some GPIO pins need to use unlocked
> >> I2C transfers for that to work with the above locking scheme. And
> >> no GPIO driver does not want to know about that at all. I.e. you
> >> have two fundamentally different requirement depending on if the
> >> GPIO pins controlling the mux are controlled using the muxed bus
> >> or if the pins are controlled in some completely unrelated way.
> >> The latter case is probably the normal case, with the GPIO mux
> >> controlled directly from some SoC pins. In the latter case you
> >> also want to prevent any transfer on the bus while the GPIO pins
> >> for the mux are changing, i.e. the total opposite of the former
> >> case. It gets really really hairy if you have multiple levels
> >> of muxes...
> >>
> >> There are a some old drivers (e.g. i2c/busses/i2c-amd756-s4882.c)
> >> that handles this by simply bypassing the GPIO subsystem, but
> >> that is not really an option if some pins are used to mux the
> >> I2C bus while some pins are used for other things.  
> > 
> > I see.
> >   
> >>
> >> I don't know if this affects I3C before muxes are added, but I
> >> suspect muxes will happen sooner rather than later, since the
> >> spec mentions that the bus only support 11 devices maximum. 11
> >> don't seem like a lot, and it seems likely that there will be
> >> a need to have more devices somehow...  
> > 
> > I can't tell, and that's the whole problem here. How can you design a
> > solution for something that does not exist yet? Fixing the I2C muxing
> > logic, if it needs to be, is something I can understand. But how can you
> > envision what I3C muxes (if they ever exist) will look like?  
> 
> Yeah, you have a point there. One problem is that I don't even see
> how the situation can be unified for I2C...

That's a problem, indeed. I guess there was discussion with the I²C
maintainers in the past, what was the outcome?

> 
> >>
> >> But just maybe, in order to not run into the above situation, it
> >> needs to be handled right from the start with preparatory and
> >> cleanup stages of each transfers, or something?  
> > 
> > How about applying this approach to I2C first and see how it flies.
> > Changing the I3C framework afterwards (when I3C muxes come in)
> > shouldn't be that complicated.  
> 
> That would require more thinking first, and I fear that the overhaul
> will be bigger than what is called for given the rather fringe cases
> that cause problems.

Ok.

> 
> Note that I'm not trying to block I3C, I'm just trying to trigger
> some thinking before the fact...

Just because the I3C framework is upstreamed doesn't mean everything is
set in stone. If we need to change the locking-scheme because a new
use case forces us to rework it, I'll be the first one to push for it,
but right now, I don't see what we can do given the information we have.

Regards,

Boris
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 09/10] gpio: Add a driver for Cadence I3C GPIO expander

[Andy Shevchenko]

On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> Add a driver for Cadence I3C GPIO expander.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

> +       scratchbuf = kmalloc(sizeof(*scratchbuf) * 2, GFP_KERNEL);

kmalloc_array() ?

> +       ret = i3c_device_do_priv_xfers(gpioc->i3cdev, xfers,
> +                                      ARRAY_SIZE(xfers));

One line?


> +static void cdns_i3c_gpio_set_multiple(struct gpio_chip *g,
> +                                      unsigned long *mask,
> +                                      unsigned long *bits)
> +{
> +       struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g);
> +       u8 newovr;
> +       int ret;
> +
> +       newovr = (gpioc->ovr & ~(*mask)) | (*bits & *mask);
> +       if (newovr == gpioc->ovr)
> +               return;
> +
> +       ret = cdns_i3c_gpio_write_reg(gpioc, OVR, newovr);
> +       if (!ret)
> +               gpioc->ovr = newovr;

You don't change mask here, why do you need a pointer to it?

> +}

> +static int cdns_i3c_gpio_get_multiple(struct gpio_chip *g,
> +                                     unsigned long *mask,
> +                                     unsigned long *bits)
> +{
> +       struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g);
> +       int ret;
> +       u8 ivr;
> +
> +       ret = cdns_i3c_gpio_read_reg(gpioc, IVR, &ivr);
> +       if (ret)
> +               return ret;
> +
> +       *bits = ivr & *mask & gpioc->dir;
> +       *bits |= gpioc->ovr & *mask & ~gpioc->dir;

Ditto.

> +
> +       return 0;
> +}

> +static void cdns_i3c_gpio_ibi_handler(struct i3c_device *i3cdev,
> +                                     const struct i3c_ibi_payload *payload)
> +{
> +       struct cdns_i3c_gpio *gpioc = i3cdev_get_drvdata(i3cdev);

> +       u8 isr = 0;

Perhaps you need to check the result of _read_reg() below instead of
dummy assignment?

> +       int i;
> +
> +       cdns_i3c_gpio_read_reg(gpioc, ISR, &isr);

> +       for (i = 0; i < 8; i++) {
> +               unsigned int irq;
> +
> +               if (!(BIT(i) & isr & gpioc->imr))
> +                       continue;

for_each_set_bit() ?

> +
> +               irq = irq_find_mapping(gpioc->gpioc.irq.domain, i);
> +               handle_nested_irq(irq);
> +       }
> +}

> +static const struct i3c_device_id cdns_i3c_gpio_ids[] = {
> +       I3C_DEVICE(0x1c9, 0x0, NULL),

> +       { /* sentinel */ },

Slightly better without comma.

> +};
> +MODULE_DEVICE_TABLE(i3c, cdns_i3c_gpio_ids);

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 09/10] gpio: Add a driver for Cadence I3C GPIO expander

[Boris Brezillon]

Hi Andy,

On Tue, 26 Jun 2018 22:07:03 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> > Add a driver for Cadence I3C GPIO expander.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>  
> 
> > +       scratchbuf = kmalloc(sizeof(*scratchbuf) * 2, GFP_KERNEL);  
> 
> kmalloc_array() ?
> 
> > +       ret = i3c_device_do_priv_xfers(gpioc->i3cdev, xfers,
> > +                                      ARRAY_SIZE(xfers));  
> 
> One line?
> 
> 
> > +static void cdns_i3c_gpio_set_multiple(struct gpio_chip *g,
> > +                                      unsigned long *mask,
> > +                                      unsigned long *bits)
> > +{
> > +       struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g);
> > +       u8 newovr;
> > +       int ret;
> > +
> > +       newovr = (gpioc->ovr & ~(*mask)) | (*bits & *mask);
> > +       if (newovr == gpioc->ovr)
> > +               return;
> > +
> > +       ret = cdns_i3c_gpio_write_reg(gpioc, OVR, newovr);
> > +       if (!ret)
> > +               gpioc->ovr = newovr;  
> 
> You don't change mask here, why do you need a pointer to it?

What are you talking about??!!! This is part of the gpio_chip interface
that drivers have to implement. You can't decide that mask should not be
a pointer. And if you actually look at the code, you'll probably see
that the reason we're passed a pointer here is that the GPIO chip might
expose more GPIOs than can be represented by an unsigned long, hence
the array of unsigned long.

I'll say it here because this is not the first time I'm pissed off by
one of your review. You seem to review everything that is posted on the
LKML, and most of the time your reviews are superficial (focused on tiny
details or coding style issues). Don't you have better things to do?

I mean, I'm fine when I get such comments from people that also do
useful comments, but yours are most of the time useless and sometime
even wrong because you didn't time to look at the code in details.

> 
> > +}  
> 
> > +static int cdns_i3c_gpio_get_multiple(struct gpio_chip *g,
> > +                                     unsigned long *mask,
> > +                                     unsigned long *bits)
> > +{
> > +       struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g);
> > +       int ret;
> > +       u8 ivr;
> > +
> > +       ret = cdns_i3c_gpio_read_reg(gpioc, IVR, &ivr);
> > +       if (ret)
> > +               return ret;
> > +
> > +       *bits = ivr & *mask & gpioc->dir;
> > +       *bits |= gpioc->ovr & *mask & ~gpioc->dir;  
> 
> Ditto.
> 
> > +
> > +       return 0;
> > +}  
> 
> > +static void cdns_i3c_gpio_ibi_handler(struct i3c_device *i3cdev,
> > +                                     const struct i3c_ibi_payload *payload)
> > +{
> > +       struct cdns_i3c_gpio *gpioc = i3cdev_get_drvdata(i3cdev);  
> 
> > +       u8 isr = 0;  
> 
> Perhaps you need to check the result of _read_reg() below instead of
> dummy assignment?
> 
> > +       int i;
> > +
> > +       cdns_i3c_gpio_read_reg(gpioc, ISR, &isr);  
> 
> > +       for (i = 0; i < 8; i++) {
> > +               unsigned int irq;
> > +
> > +               if (!(BIT(i) & isr & gpioc->imr))
> > +                       continue;  
> 
> for_each_set_bit() ?
> 
> > +
> > +               irq = irq_find_mapping(gpioc->gpioc.irq.domain, i);
> > +               handle_nested_irq(irq);
> > +       }
> > +}  
> 
> > +static const struct i3c_device_id cdns_i3c_gpio_ids[] = {
> > +       I3C_DEVICE(0x1c9, 0x0, NULL),  
> 
> > +       { /* sentinel */ },  
> 
> Slightly better without comma.
> 
> > +};
> > +MODULE_DEVICE_TABLE(i3c, cdns_i3c_gpio_ids);  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 09/10] gpio: Add a driver for Cadence I3C GPIO expander

[Andy Shevchenko]

On Tue, Jun 26, 2018 at 10:56 PM, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> On Tue, 26 Jun 2018 22:07:03 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon
>> <boris.brezillon@bootlin.com> wrote:
>> > Add a driver for Cadence I3C GPIO expander.
>> >
>> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
>>
>> > +       scratchbuf = kmalloc(sizeof(*scratchbuf) * 2, GFP_KERNEL);
>>
>> kmalloc_array() ?
>>
>> > +       ret = i3c_device_do_priv_xfers(gpioc->i3cdev, xfers,
>> > +                                      ARRAY_SIZE(xfers));
>>
>> One line?
>>

>> You don't change mask here, why do you need a pointer to it?
>
> What are you talking about??!!! This is part of the gpio_chip interface
> that drivers have to implement. You can't decide that mask should not be
> a pointer. And if you actually look at the code, you'll probably see
> that the reason we're passed a pointer here is that the GPIO chip might
> expose more GPIOs than can be represented by an unsigned long, hence
> the array of unsigned long.
>

> I'll say it here because this is not the first time I'm pissed off by
> one of your review.

Like 'I like offending people, because I think people who get offended
should be offended.' ?
I'm not that guy, relax.

>  You seem to review everything that is posted on the LKML,

That's not true.

> and most of the time your reviews are superficial (focused on tiny
> details or coding style issues). Don't you have better things to do?

If your code is written using ancient style and / or API it's not good
to start with.
Isn't it? Otherwise, why we do introduce new internal APIs, for what purpose?

On top of that you might find more interesting reviews where I pointed
out to a memory leak or other nasty stuff. But of course you prefer
not to norice that.
I understand you.

> I mean, I'm fine when I get such comments from people that also do
> useful comments, but yours are most of the time useless and sometime
> even wrong because you didn't time to look at the code in details.

Calm down, please.
You would simple ignore them. Your choice.

But okay, I would try to avoid your stuff to make you happier. Sorry
for disturbing.

>> Ditto.

In these two comments I'm indeed wrong.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 02/10] docs: driver-api: Add I3C documentation

[Randy Dunlap]

Hi,

On 06/22/2018 03:49 AM, Boris Brezillon wrote:
> Add the I3C documentation describing the protocol, the master driver API
> and the device driver API.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
> Changes in v5:
> - Remove useless conf.py file
> - Add SPDX headers
> 
> Changes in v2:
> - Moved out of patch "i3c: Add core I3C infrastructure"
> - Add link to the I3C spec
> - Move rst files in Documentation/driver-api/i3c/
> ---
>  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 +
>  5 files changed, 234 insertions(+)
>  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


> diff --git a/Documentation/driver-api/i3c/protocol.rst b/Documentation/driver-api/i3c/protocol.rst
> new file mode 100644
> index 000000000000..a768afa7f12a
> --- /dev/null
> +++ b/Documentation/driver-api/i3c/protocol.rst
> @@ -0,0 +1,203 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +============
> +I3C protocol
> +============
> +
> +Disclaimer
> +==========
> +
> +This chapter will focus on aspects that matter to software developers. For
> +everything hardware related (like how things are transmitted on the bus, how
> +collisions are prevented, ...) please have a look at the I3C specification.
> +
> +This document is just a brief introduction to the I3C protocol and the concepts
> +it brings on the table. If you need more information, please refer to the MIPI

      brings to the table.

> +I3C specification (can be downloaded here
> +http://resources.mipi.org/mipi-i3c-v1-download).
> +
> +Introduction
> +============
> +
> +The I3C (pronounced 'eye-three-see') is a MIPI standardized protocol designed
> +to overcome I2C limitations (limited speed, external signals needed for
> +interrupts, no automatic detection of the devices connected to the bus, ...)
> +while remaining power-efficient.
> +
> +I3C Bus
> +=======
> +
> +An I3C bus is made of several I3C devices and possibly some I2C devices as
> +well, but let's focus on I3C devices for now.
> +
> +An I3C device on the I3C bus can have one of the following roles:
> +
> +* Master: the device is driving the bus. It's the one in charge of initiating
> +  transactions or deciding who is allowed to talk on the bus (slave generated
> +  events are possible in I3C, see below).
> +* Slave: the device acts as a slave, and is not able to send frames to another
> +  slave on the bus. The device can still send events to the master on
> +  its own initiative if the master allowed it.
> +
> +I3C is a multi-master protocol, so there might be several masters on a bus,
> +though only one device can act as a master at a given time. In order to gain
> +bus ownership, a master has to follow a specific procedure.
> +
> +Each device on the I3C bus has to be assigned a dynamic address to be able to
> +communicate. Until this is done, the device should only respond to a limited
> +set of commands. If it has a static address (also called legacy I2C address),
> +the device can reply to I2C transfers.
> +
> +In addition to these per-device addresses, the protocol defines a broadcast
> +address in order to address all devices on the bus.
> +
> +Once a dynamic address has been assigned to a device, this address will be used
> +for any direct communication with the device. Note that even after being
> +assigned a dynamic address, the device should still process broadcast messages.
> +
> +I3C Device discovery
> +====================
> +
> +The I3C protocol defines a mechanism to automatically discover devices present
> +on the bus, their capabilities and the functionalities they provide. In this
> +regard I3C is closer to a discoverable bus like USB than it is to I2C or SPI.
> +
> +The discovery mechanism is called DAA (Dynamic Address Assignment), because it
> +not only discovers devices but also assigns them a dynamic address.
> +
> +During DAA, each I3C device reports 3 important things:
> +
> +* BCR: Bus Characteristic Register. This 8-bit register describes the device bus
> +  related capabilities
> +* DCR: Device Characteristic Register. This 8-bit register describes the
> +  functionalities provided by the device
> +* Provisional ID: A 48-bit unique identifier. On a given bus there should be no
> +  Provisional ID collision, otherwise the discovery mechanism may fail.
> +
> +I3C slave events
> +================
> +
> +The I3C protocol allows slaves to generate events on their own, and thus allows
> +them to take temporary control of the bus.
> +
> +This mechanism is called IBI for In Band Interrupts, and as stated in the name,
> +it allows devices to generate interrupts without requiring an external signal.
> +
> +During DAA, each device on the bus has been assigned an address, and this
> +address will serve as a priority identifier to determine who wins if 2 different
> +devices are generating an interrupt at the same moment on the bus (the lower the
> +dynamic address the higher the priority).
> +
> +Masters are allowed to inhibit interrupts if they want to. This inhibition
> +request can be broadcasted (applies to all devices) or sent to a specific

           can be broadcast

> +device.
> +
> +I3C Hot-Join
> +============
> +
> +The Hot-Join mechanism is similart to USB hotplug. This mechanism allows

                             similar

> +slaves to join the bus after it has been initialized by the master.
> +
> +This covers the following use cases:
> +
> +* the device is not powered when the bus is probed
> +* the device is hotplugged on the bus through an extension board
> +
> +This mechanism is relying on slave events to inform the master that a new
> +device joined the bus and is waiting for a dynamic address.
> +
> +The master is then free to address the request as it wishes: ignore it or
> +assign a dynamic address to the slave.
> +
> +I3C transfer types
> +==================
> +
> +If you omit SMBus (which is just a standardization on how to access registers
> +exposed by I2C devices), I2C has only one transfer type.
> +
> +I3C defines 3 different classes of transfer in addition to I2C transfers which
> +are here for backward compatibility with I2C devices.
> +
> +I3C CCC commands
> +----------------
> +
> +CCC (Common Command Code) commands are meant to be used for anything that is
> +related to bus management and all features that are common to a set of devices.
> +
> +CCC commands contain an 8-bit CCC id describing the command that is executed.

preferably s/id/ID/

> +The MSB of this id specifies whether this is a broadcast command (bit7 = 0) or a

ditto, s/id/ID/

> +unicast one (bit7 = 1).
> +
> +The command ID can be followed by a payload. Depending on the command, this
> +payload is either sent by the master sending the command (write CCC command),
> +or sent by the slave receiving the command (read CCC command). Of course, read
> +accesses only apply to unicast commands.
> +Note that, when sending a CCC command to a specific device, the device address
> +is passed in the first byte of the payload.
> +
> +The payload length is not explicitly passed on the bus, and should be extracted
> +from the CCC id.

s/id/ID/

> +
> +Note that vendors can use a dedicated range of CCC ids for their own commands

                                                      IDs

> +(0x61-0x7f and 0xe0-0xef).
> +
> +I3C Private SDR transfers
> +-------------------------
> +
> +Private SDR (Single Data Rate) transfers should be used for anything that is
> +device specific and does not require high transfer speed.
> +
> +It is the equivalent of I2C transfers but in the I3C world. Each transfer is
> +passed the device address (dynamic address assigned during DAA), a payload
> +and a direction.
> +
> +The only difference with I2C is that the transfer is much faster (typical SCL

what is SCL?  It's not used anywhere else in this doc.

> +frequency is 12.5MHz).
> +
> +I3C HDR commands
> +----------------
> +
> +HDR commands should be used for anything that is device specific and requires
> +high transfer speed.
> +
> +The first thing attached to an HDR command is the HDR mode. There are currently
> +3 different modes defined by the I3C specification (refer to the specification
> +for more details):
> +
> +* HDR-DDR: Double Data Rate mode
> +* HDR-TSP: Ternary Symbol Pure. Only usable on busses with no I2C devices
> +* HDR-TSL: Ternary Symbol Legacy. Usable on busses with I2C devices
> +
> +When sending an HDR command, the whole bus has to enter HDR mode, which is done
> +using a broadcast CCC command.
> +Once the bus has entered a specific HDR mode, the master sends the HDR command.
> +An HDR command is made of:
> +
> +* one 16-bits command word
> +* N 16-bits data words

I supposed the I3C spec will tell me the byte order of these words on the bus?
or this doc could tell us here.

> +
> +Those words may be wrapped with specific preambles/post-ambles which depend on
> +the chosen HDR mode and are detailed here (see the specification for more
> +details).
> +
> +The 16-bits command word is made of:
> +
> +* bit[15]: direction bit, read is 1 write is 0

                             read is 1, write is 0

> +* bit[14:8]: command code. Identifies the command being executed, the amount of
> +  data words and their meaning
> +* bit[7:1]: I3C address of the device this command is addressed to
> +* bit[0]: reserved/parity-bit
> +
> +Backward compatibility with I2C devices
> +=======================================
> +
> +The I3C protocol has been designed to be backward compatible with I2C devices.
> +This backward compatibility allows one to connect a mix of I2C and I3C devices
> +on the same bus, though, in order to be really efficient, I2C devices should
> +be equipped with 50 ns spike filters.
> +
> +I2C devices can't be discovered like I3C ones and have to be statically
> +declared. In order to let the master know what these devices are capable of
> +(both in terms of bus related limitations and functionalities), the software
> +has to provide some information, which is done through the LVR (Legacy I2C
> +Virtual Register).

and you can add (if you want to):
Reviewed-by: Randy Dunlap <rdunlap@infradead.org>

thanks,
-- 
~Randy
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 09/10] gpio: Add a driver for Cadence I3C GPIO expander

[Boris Brezillon]

On Tue, 26 Jun 2018 23:44:36 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Jun 26, 2018 at 10:56 PM, Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> > On Tue, 26 Jun 2018 22:07:03 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> >> On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon
> >> <boris.brezillon@bootlin.com> wrote:  
> >> > Add a driver for Cadence I3C GPIO expander.
> >> >
> >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>  
> >>  
> >> > +       scratchbuf = kmalloc(sizeof(*scratchbuf) * 2, GFP_KERNEL);  
> >>
> >> kmalloc_array() ?
> >>  
> >> > +       ret = i3c_device_do_priv_xfers(gpioc->i3cdev, xfers,
> >> > +                                      ARRAY_SIZE(xfers));  
> >>
> >> One line?
> >>  
> 
> >> You don't change mask here, why do you need a pointer to it?  
> >
> > What are you talking about??!!! This is part of the gpio_chip interface
> > that drivers have to implement. You can't decide that mask should not be
> > a pointer. And if you actually look at the code, you'll probably see
> > that the reason we're passed a pointer here is that the GPIO chip might
> > expose more GPIOs than can be represented by an unsigned long, hence
> > the array of unsigned long.
> >  
> 
> > I'll say it here because this is not the first time I'm pissed off by
> > one of your review.  
> 
> Like 'I like offending people, because I think people who get offended
> should be offended.' ?
> I'm not that guy, relax.

I'm not offended, just annoyed, and not because I have things to change
in my patchset (I'm used to that and that's something I comply with
most of the time), but because the only reviews I had from you were of
this kind (nitpicking on minor stuff).

> 
> >  You seem to review everything that is posted on the LKML,  
> 
> That's not true.
> 
> > and most of the time your reviews are superficial (focused on tiny
> > details or coding style issues). Don't you have better things to do?  
> 
> If your code is written using ancient style and / or API it's not good
> to start with.
> Isn't it? Otherwise, why we do introduce new internal APIs, for what purpose?

Come on!

- kzalloc() vs kmalloc_array()
- for (i = 0; i < n, i++) { if (BIT(x) & var) ... }
  vs
  for_each_bit_set() (which is not even applicable here because I'm
  not manipulating an unsigned long)

Where do you see ancient style APIs here?

And that's not even the problem. I'm okay to fix those things, but you
only ever do these kind of reviews, and sometime you even act like you
know better than the developer or the maintainer how the code should be
organized without even digging enough to have a clue (your comment on
the fact that mask should not be a pointer is a good example of such
situations).

> 
> On top of that you might find more interesting reviews where I pointed
> out to a memory leak or other nasty stuff. But of course you prefer
> not to norice that.

Yes, sometime you have valid point, but it gets lost in all the noise.

> I understand you.
> 
> > I mean, I'm fine when I get such comments from people that also do
> > useful comments, but yours are most of the time useless and sometime
> > even wrong because you didn't time to look at the code in details.  
> 
> Calm down, please.

Why? Because I say you didn't look at the code in details? Isn't it
true? Did you look at what I3C is, how it works or how the I3C framework
is architectured? Because that's the kind of reviews I'd love to have on
this series.

> You would simple ignore them. Your choice.

That's not my point. My point is, maybe you should do less reviews but
spend more time on each of them so you don't just scratch the surface
with comments I could get from a tool like checkpatch.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 02/10] docs: driver-api: Add I3C documentation

[Boris Brezillon]

Hi Randy,

On Tue, 26 Jun 2018 14:07:49 -0700
Randy Dunlap <rdunlap@infradead.org> wrote:


> > +
> > +I3C Private SDR transfers
> > +-------------------------
> > +
> > +Private SDR (Single Data Rate) transfers should be used for anything that is
> > +device specific and does not require high transfer speed.
> > +
> > +It is the equivalent of I2C transfers but in the I3C world. Each transfer is
> > +passed the device address (dynamic address assigned during DAA), a payload
> > +and a direction.
> > +
> > +The only difference with I2C is that the transfer is much faster (typical SCL  
> 
> what is SCL?  It's not used anywhere else in this doc.

It's an acronym used by I²C, it means Serial Clock Line. I'll just
replace that by "typical clock frequency is 12.5MHz".

> 
> > +frequency is 12.5MHz).
> > +
> > +I3C HDR commands
> > +----------------
> > +
> > +HDR commands should be used for anything that is device specific and requires
> > +high transfer speed.
> > +
> > +The first thing attached to an HDR command is the HDR mode. There are currently
> > +3 different modes defined by the I3C specification (refer to the specification
> > +for more details):
> > +
> > +* HDR-DDR: Double Data Rate mode
> > +* HDR-TSP: Ternary Symbol Pure. Only usable on busses with no I2C devices
> > +* HDR-TSL: Ternary Symbol Legacy. Usable on busses with I2C devices
> > +
> > +When sending an HDR command, the whole bus has to enter HDR mode, which is done
> > +using a broadcast CCC command.
> > +Once the bus has entered a specific HDR mode, the master sends the HDR command.
> > +An HDR command is made of:
> > +
> > +* one 16-bits command word
> > +* N 16-bits data words  
> 
> I supposed the I3C spec will tell me the byte order of these words on the bus?
> or this doc could tell us here.

It's big endian. I'll make it clear in this doc.

I'll also fix all the other mistakes you pointed out.

> 
> and you can add (if you want to):
> Reviewed-by: Randy Dunlap <rdunlap@infradead.org>

I'll definitely add your R-b. Thanks for the review.

Boris
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 09/10] gpio: Add a driver for Cadence I3C GPIO expander

[Andy Shevchenko]

On Wed, Jun 27, 2018 at 12:46 AM, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> On Tue, 26 Jun 2018 23:44:36 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Tue, Jun 26, 2018 at 10:56 PM, Boris Brezillon
>> <boris.brezillon@bootlin.com> wrote:
>> > On Tue, 26 Jun 2018 22:07:03 +0300
>> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> >> On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon
>> >> <boris.brezillon@bootlin.com> wrote:

>> > I'll say it here because this is not the first time I'm pissed off by
>> > one of your review.
>>
>> Like 'I like offending people, because I think people who get offended
>> should be offended.' ?
>> I'm not that guy, relax.
>
> I'm not offended, just annoyed, and not because I have things to change
> in my patchset (I'm used to that and that's something I comply with
> most of the time), but because the only reviews I had from you were of
> this kind (nitpicking on minor stuff).

OK, point taken.

>> > and most of the time your reviews are superficial (focused on tiny
>> > details or coding style issues). Don't you have better things to do?
>>
>> If your code is written using ancient style and / or API it's not good
>> to start with.
>> Isn't it? Otherwise, why we do introduce new internal APIs, for what purpose?
>
> Come on!
>
> - kzalloc() vs kmalloc_array()
> - for (i = 0; i < n, i++) { if (BIT(x) & var) ... }
>   vs
>   for_each_bit_set() (which is not even applicable here because I'm
>   not manipulating an unsigned long)
>
> Where do you see ancient style APIs here?

Both. kmalloc_array() and for_each_set_bit() were introduced quite
long time ago.

On top of that you are open coding, if I'm not mistaken, cpu_to_be32()
and be32_to_cpu().
...and more I believe.

> And that's not even the problem. I'm okay to fix those things, but you
> only ever do these kind of reviews, and sometime you even act like you
> know better than the developer or the maintainer how the code should be
> organized without even digging enough to have a clue (your comment on
> the fact that mask should not be a pointer is a good example of such
> situations).

"sometime". Yes, I admit my mistakes.

>> On top of that you might find more interesting reviews where I pointed
>> out to a memory leak or other nasty stuff. But of course you prefer
>> not to norice that.
>
> Yes, sometime you have valid point, but it gets lost in all the noise.

Btw, you forgot to call of_node_put() on error path in one case. Did I
again missed the details?

>> > I mean, I'm fine when I get such comments from people that also do
>> > useful comments, but yours are most of the time useless and sometime
>> > even wrong because you didn't time to look at the code in details.
>>
>> Calm down, please.
>
> Why? Because I say you didn't look at the code in details? Isn't it
> true? Did you look at what I3C is, how it works or how the I3C framework
> is architectured? Because that's the kind of reviews I'd love to have on
> this series.

You got me.
I have a copy of the spec, this require a bit of time to go through.

For now I can offer you to consider IBI implementation using IRQ chip framework.
It would give few advantages (like we do this for GPIO), such as IRQ
statistics or wake up capabilities.

>> You would simple ignore them. Your choice.
>
> That's not my point. My point is, maybe you should do less reviews but
> spend more time on each of them

Good point.

> so you don't just scratch the surface
> with comments I could get from a tool like checkpatch.

Perhaps you should run checkpatch yourself then?

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 09/10] gpio: Add a driver for Cadence I3C GPIO expander

[Boris Brezillon]

Hi Andy,

On Wed, 27 Jun 2018 20:53:51 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Jun 27, 2018 at 12:46 AM, Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> > On Tue, 26 Jun 2018 23:44:36 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> >> On Tue, Jun 26, 2018 at 10:56 PM, Boris Brezillon
> >> <boris.brezillon@bootlin.com> wrote:  
> >> > On Tue, 26 Jun 2018 22:07:03 +0300
> >> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> >> >> On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon
> >> >> <boris.brezillon@bootlin.com> wrote:  
> 
> >> > I'll say it here because this is not the first time I'm pissed off by
> >> > one of your review.  
> >>
> >> Like 'I like offending people, because I think people who get offended
> >> should be offended.' ?
> >> I'm not that guy, relax.  
> >
> > I'm not offended, just annoyed, and not because I have things to change
> > in my patchset (I'm used to that and that's something I comply with
> > most of the time), but because the only reviews I had from you were of
> > this kind (nitpicking on minor stuff).  
> 
> OK, point taken.
> 
> >> > and most of the time your reviews are superficial (focused on tiny
> >> > details or coding style issues). Don't you have better things to do?  
> >>
> >> If your code is written using ancient style and / or API it's not good
> >> to start with.
> >> Isn't it? Otherwise, why we do introduce new internal APIs, for what purpose?  
> >
> > Come on!
> >
> > - kzalloc() vs kmalloc_array()
> > - for (i = 0; i < n, i++) { if (BIT(x) & var) ... }
> >   vs
> >   for_each_bit_set() (which is not even applicable here because I'm
> >   not manipulating an unsigned long)
> >
> > Where do you see ancient style APIs here?  
> 
> Both. kmalloc_array() and for_each_set_bit() were introduced quite
> long time ago.

I mean, kzalloc() is not deprecated AFAIK and I don't really see the
benefit of using kmalloc_array(), but if that makes you happy, let's go
for kmalloc_array().

For for_each_bit_set() it would require changing the type of isr +
having a temporary variable to get the reg value into an u8. Again, I
can do the change, but I don't really see how it improves the code. 

> 
> On top of that you are open coding, if I'm not mistaken, cpu_to_be32()
> and be32_to_cpu().

Care to point where?

> ...and more I believe.

Care to point what?
 
> >> On top of that you might find more interesting reviews where I pointed
> >> out to a memory leak or other nasty stuff. But of course you prefer
> >> not to norice that.  
> >
> > Yes, sometime you have valid point, but it gets lost in all the noise.  
> 
> Btw, you forgot to call of_node_put() on error path in one case.

In this driver or another patch of the series? I don't see any
of_node_get() call in this GPIO driver, but maybe it's something done
in one of the GPIO helper.

> Did I
> again missed the details?
> 
> >> > I mean, I'm fine when I get such comments from people that also do
> >> > useful comments, but yours are most of the time useless and sometime
> >> > even wrong because you didn't time to look at the code in details.  
> >>
> >> Calm down, please.  
> >
> > Why? Because I say you didn't look at the code in details? Isn't it
> > true? Did you look at what I3C is, how it works or how the I3C framework
> > is architectured? Because that's the kind of reviews I'd love to have on
> > this series.  
> 
> You got me.
> I have a copy of the spec, this require a bit of time to go through.

Cool!

> 
> For now I can offer you to consider IBI implementation using IRQ chip framework.
> It would give few advantages (like we do this for GPIO), such as IRQ
> statistics or wake up capabilities.

Just pasting the comment I made in my cover letter:

"
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. 
"

To sum-up, yes I tried implementing what you suggest, and it ended
being messy for 2 main reasons:

1/ IBIs have payload attached to them, and exposing IBIs are regular
IRQs meant adding a payload queuing mechanism to the i3c_device so that
the I3C device driver could dequeue each payload independently. Clearly
not impossible to deal with, but conceptually far from the concept of
IRQs we have in Linux.

2/ The I3C APIs are meant to be used in non-atomic context, but if you
expose IBIs are regular IRQs, the irqchip will have to mask/unmask the
IRQs from an atomic context, and masking/unmasking IRQs almost always
implies doing a CCC or priv SDR transfer. Plus, most of the time you'll
have to retrieve extra info from the IRQ handler, which again means
sending frames on the I3C bus. We could make that work by either
supporting async transfers or having the irq chip handle the IRQ
handlers from a thread. But both options add extra complexity for no
real benefit given that IBIs are already not exactly fitting in the
Linux IRQ model.

The discussion I had with Mark Zyngier finished convincing me that
IBIs would be better exposed with their own API.

> 
> >> You would simple ignore them. Your choice.  
> >
> > That's not my point. My point is, maybe you should do less reviews but
> > spend more time on each of them  
> 
> Good point.
> 
> > so you don't just scratch the surface
> > with comments I could get from a tool like checkpatch.  
> 
> Perhaps you should run checkpatch yourself then?
> 

I do run checkpatch --strict and fix most of the thing reported except
those hurting readability. I don't remember seeing checkpatch complain
about kzalloc() usage, and I guess it's not smart enough to detect that
for_each_bit_set() can be used to replace the "for() if (BIT(x) & val)"
pattern.

Anyway, thanks for having a look at the I3C spec and starting the
discussion on IBIs. I guess I owe you an apology.

Regards,

Boris
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 09/10] gpio: Add a driver for Cadence I3C GPIO expander

[Linus Walleij]

On Fri, Jun 22, 2018 at 12:49 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:

> Add a driver for Cadence I3C GPIO expander.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

If you need to merge this through the subsystem merge.

I see there was some heat in this discussion between you
and Andy, and it seems to be resolved. Which is good
because you are both senior contributors that I value
a lot.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 09/10] gpio: Add a driver for Cadence I3C GPIO expander

[Joe Perches]

On Wed, 2018-06-27 at 21:36 +0200, Boris Brezillon wrote:
> I mean, kzalloc() is not deprecated AFAIK and I don't really see the
> benefit of using kmalloc_array(), but if that makes you happy, let's go
> for kmalloc_array().

kcalloc

> I do run checkpatch --strict and fix most of the thing reported except
> those hurting readability. I don't remember seeing checkpatch complain
> about kzalloc() usage, and I guess it's not smart enough to detect that
> for_each_bit_set() can be used to replace the "for() if (BIT(x) & val)"
> pattern.

That would not an appropriate conversion suggestion in any case.
coccinelle could at least look at whether or not x is allocated
as a bitmap via DECLARE_BITMAP or bitmap_alloc


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 09/10] gpio: Add a driver for Cadence I3C GPIO expander

[Andy Shevchenko]

On Thu, Jun 28, 2018 at 1:54 AM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2018-06-27 at 21:36 +0200, Boris Brezillon wrote:
>> I mean, kzalloc() is not deprecated AFAIK and I don't really see the
>> benefit of using kmalloc_array(), but if that makes you happy, let's go
>> for kmalloc_array().
>
> kcalloc

IIRC in the original code kmalloc(x*y) is used.

>> I do run checkpatch --strict and fix most of the thing reported except
>> those hurting readability. I don't remember seeing checkpatch complain
>> about kzalloc() usage, and I guess it's not smart enough to detect that
>> for_each_bit_set() can be used to replace the "for() if (BIT(x) & val)"
>> pattern.
>
> That would not an appropriate conversion suggestion in any case.
> coccinelle could at least look at whether or not x is allocated
> as a bitmap via DECLARE_BITMAP or bitmap_alloc

Huh?!

bitmap operations are working against unsigned long *. one long is
also a bitmap.
So, that coccinelle scripts must be fixed accordingly.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 09/10] gpio: Add a driver for Cadence I3C GPIO expander

[Joe Perches]

On Thu, 2018-06-28 at 03:00 +0300, Andy Shevchenko wrote:
> bitmap operations are working against unsigned long *. one long is
> also a bitmap.

Should only be so if declared via DECLARE_BITMAP

> So, that coccinelle scripts must be fixed accordingly.

We disagree about a non-existent script.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 09/10] gpio: Add a driver for Cadence I3C GPIO expander

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 185 bytes --]


> I see there was some heat in this discussion between you
> and Andy, and it seems to be resolved. Which is good
> because you are both senior contributors that I value
> a lot.

+1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 01/10] i3c: Add core I3C infrastructure

[vitor]

Hi Boris,


On 22-06-2018 11:49, Boris Brezillon wrote:
> +static int of_i3c_master_add_i3c_dev(struct i3c_master_controller *master,
> +				     struct device_node *node, u32 *reg)
> +{
> +	struct i3c_device_info info = { };
> +	enum i3c_addr_slot_status addrstatus;
> +	struct i3c_device *i3cdev;
> +	u32 init_dyn_addr = 0;
> +
> +	if (reg[0]) {
> +		if (reg[0] > I3C_MAX_ADDR)
> +			return -EINVAL;
> +
> +		addrstatus = i3c_bus_get_addr_slot_status(master->bus, reg[0]);
> +		if (addrstatus != I3C_ADDR_SLOT_FREE)
> +			return -EINVAL;
> +	}
> +
> +	info.static_addr = reg[0];
> +
> +	if (!of_property_read_u32(node, "assigned-address", &init_dyn_addr)) {
> +		if (init_dyn_addr > I3C_MAX_ADDR)
> +			return -EINVAL;
> +
> +		addrstatus = i3c_bus_get_addr_slot_status(master->bus,
> +							  init_dyn_addr);
> +		if (addrstatus != I3C_ADDR_SLOT_FREE)
> +			return -EINVAL;
> +	}
> +
> +	info.pid = ((u64)reg[1] << 32) | reg[2];
> +
> +	if ((info.pid & GENMASK_ULL(63, 48)) ||
> +	    I3C_PID_RND_LOWER_32BITS(info.pid))
> +		return -EINVAL;
> +
> +	i3cdev = i3c_master_alloc_i3c_dev(master, &info, &i3c_device_type);
> +	if (IS_ERR(i3cdev))
> +		return PTR_ERR(i3cdev);
> +
> +	i3cdev->init_dyn_addr = init_dyn_addr;
> +	i3cdev->dev.of_node = node;
> +	list_add_tail(&i3cdev->common.node, &master->bus->devs.i3c);
> +
> +	return 0;
> +}
> +

I'm writing the driver for the Synopsys master and but now I getting an 
issue.

I use the "slot" of the device to do all transfers, something like you 
use in DAA. I using the master_priv to save the "slot" per device but 
the problem is when I call the i3c_master_add_i3c_dev_locked() to 
retrieve the info I don't have it yet.

 From my analysis this can be solve with:
     - send PID, BCR and DCR when I call i3c_master_add_i3c_dev_locked() 
or similar function.
     - Pre-allocate an i3c_device -> attach it (slot data goes to 
master_priv) -> retrieve info -> if there is already an i3c_device with 
same PID destroy the pre-allocated one.
     - Replace the info.dyn_address with a structure with dyn_address 
and slot and use it in CCC structure.

This is something that will need to be supported for I3C HCI spec too. 
Do you have any suggestion?

Best regards,
Vitor Soares


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 01/10] i3c: Add core I3C infrastructure

[Boris Brezillon]

Hi Vitor,

On Thu, 28 Jun 2018 16:38:56 +0100
vitor <Vitor.Soares@synopsys.com> wrote:

> Hi Boris,
> 
> 
> On 22-06-2018 11:49, Boris Brezillon wrote:
> > +static int of_i3c_master_add_i3c_dev(struct i3c_master_controller *master,
> > +				     struct device_node *node, u32 *reg)
> > +{
> > +	struct i3c_device_info info = { };
> > +	enum i3c_addr_slot_status addrstatus;
> > +	struct i3c_device *i3cdev;
> > +	u32 init_dyn_addr = 0;
> > +
> > +	if (reg[0]) {
> > +		if (reg[0] > I3C_MAX_ADDR)
> > +			return -EINVAL;
> > +
> > +		addrstatus = i3c_bus_get_addr_slot_status(master->bus, reg[0]);
> > +		if (addrstatus != I3C_ADDR_SLOT_FREE)
> > +			return -EINVAL;
> > +	}
> > +
> > +	info.static_addr = reg[0];
> > +
> > +	if (!of_property_read_u32(node, "assigned-address", &init_dyn_addr)) {
> > +		if (init_dyn_addr > I3C_MAX_ADDR)
> > +			return -EINVAL;
> > +
> > +		addrstatus = i3c_bus_get_addr_slot_status(master->bus,
> > +							  init_dyn_addr);
> > +		if (addrstatus != I3C_ADDR_SLOT_FREE)
> > +			return -EINVAL;
> > +	}
> > +
> > +	info.pid = ((u64)reg[1] << 32) | reg[2];
> > +
> > +	if ((info.pid & GENMASK_ULL(63, 48)) ||
> > +	    I3C_PID_RND_LOWER_32BITS(info.pid))
> > +		return -EINVAL;
> > +
> > +	i3cdev = i3c_master_alloc_i3c_dev(master, &info, &i3c_device_type);
> > +	if (IS_ERR(i3cdev))
> > +		return PTR_ERR(i3cdev);
> > +
> > +	i3cdev->init_dyn_addr = init_dyn_addr;
> > +	i3cdev->dev.of_node = node;
> > +	list_add_tail(&i3cdev->common.node, &master->bus->devs.i3c);
> > +
> > +	return 0;
> > +}
> > +  
> 
> I'm writing the driver for the Synopsys master and but now I getting an 
> issue.
> 
> I use the "slot" of the device to do all transfers, something like you 
> use in DAA. I using the master_priv to save the "slot" per device but 
> the problem is when I call the i3c_master_add_i3c_dev_locked() to 
> retrieve the info I don't have it yet.

Hm, I knew that might become a problem at some point. The Cadence IP
does not need the slot index because it works with addresses and
figure the device slot out of this address, but it looks like others
don't go this road. 

> 
>  From my analysis this can be solve with:
>      - send PID, BCR and DCR when I call i3c_master_add_i3c_dev_locked() 
> or similar function.

Except these are not the only thing we retrieve before attaching the
device. Also, if we go this road, that means we don't have the same path
for devices whose dynamic address is assigned through SETDASA, and those
that are discovered using DAA.

>      - Pre-allocate an i3c_device -> attach it (slot data goes to 
> master_priv) -> retrieve info -> if there is already an i3c_device with 
> same PID destroy the pre-allocated one.

That's the very reason I didn't go this road. It gets messy if we
already know this device. This being said, among all the options you
list here, this is the one I prefer. Let's see if we can standardize
the resource allocation/free process and let ->attach/detach() only
take care of the device/bus configuration.

>      - Replace the info.dyn_address with a structure with dyn_address 
> and slot and use it in CCC structure.

I'd really like to keep the device-slot-id a priv information, because
we don't know how other IPs will deal with I3C device resources.

> 
> This is something that will need to be supported for I3C HCI spec too. 

I agree.

> Do you have any suggestion?

I'll try to come up with something. Need to think a bit more about it.

Thanks,

Boris
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 01/10] i3c: Add core I3C infrastructure

[Arnd Bergmann]

On Fri, Jun 22, 2018 at 12:49 PM, Boris Brezillon
<boris.brezillon@bootlin.com> 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..

I just realized I replied to the wrong version when I reviewed v4 of this
patch. I think most of my comments still apply, so please see

https://lore.kernel.org/lkml/CAK8P3a1aZXf2sQW2mgwJScycKPhdoOAwxRjm5cQG83513uc3fg@mail.gmail.com/T/#u

and ignore anything that has changed in the meantime.

     Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 04/10] dt-bindings: i3c: Document core bindings

[Arnd Bergmann]

On Fri, Jun 22, 2018 at 12:49 PM, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> A new I3C subsystem has been added and a generic description has been
> created to represent the I3C bus and the devices connected on it.
>

> +I2C devices
> +===========
> +
> +Each I2C device connected to the bus should be described in a subnode. All
> +properties described in Documentation/devicetree/bindings/i2c/i2c.txt are
> +valid here, but several new properties have been added.
> +
> +New constraint on existing properties:
> +--------------------------------------
...
> +  + third cell: should be 0

What for? Just to be future-proof, or do you have something specific
in mind here?

      Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 07/10] i3c: master: Add driver for Cadence IP

[Arnd Bergmann]

On Fri, Jun 22, 2018 at 12:49 PM, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> Add a driver for Cadence I3C master IP.

The driver seems very well-written and shows that the framework got that side
of the interface right. Just one thing I noticed:

> +
> +static void cdns_i3c_master_handle_ibi(struct cdns_i3c_master *master,
> +                                      u32 ibir)

> +
> +       for (i = 0; i < IBIR_XFER_BYTES(ibir); i += 4) {
> +               u32 tmp = readl(master->regs + IBI_DATA_FIFO);
> +
> +               for (j = 0; j < 4 && i + j < dev->ibi->max_payload_len; j++)
> +                       buf[i + j] = tmp >> (j * 8);
> +       }

This seems to be a rather inefficient way to open-code a readsl().
I suppose you need to handle length that is not a multiple of 4, right?

Maybe do it like

      size_t length = IBIR_XFER_BYTES(ibir);
      readsl(master->regs + IBI_DATA_FIFO, buf, length & ~3);
      if (length & 3) {
            u32 tmp = __raw_readl(master->regs + IBI_DATA_FIFO);
            memcpy(buf + length & ~3, length & 3);
      }

     Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 04/10] dt-bindings: i3c: Document core bindings

[Boris Brezillon]

On Wed, 11 Jul 2018 16:10:10 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Fri, Jun 22, 2018 at 12:49 PM, Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> > A new I3C subsystem has been added and a generic description has been
> > created to represent the I3C bus and the devices connected on it.
> >  
> 
> > +I2C devices
> > +===========
> > +
> > +Each I2C device connected to the bus should be described in a subnode. All
> > +properties described in Documentation/devicetree/bindings/i2c/i2c.txt are
> > +valid here, but several new properties have been added.
> > +
> > +New constraint on existing properties:
> > +--------------------------------------  
> ...
> > +  + third cell: should be 0  
> 
> What for? Just to be future-proof, or do you have something specific
> in mind here?

Even if I2C devices only need 1 cell, I3C devices need 3 (1 for the
static address and 2 for the PID). Since both type of devices are
described under the same bus node, and we can have different
#address-cells, I had to put 0-padding in the last cell.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 04/10] dt-bindings: i3c: Document core bindings

[Arnd Bergmann]

On Wed, Jul 11, 2018 at 4:45 PM, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> On Wed, 11 Jul 2018 16:10:10 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:
>
>> On Fri, Jun 22, 2018 at 12:49 PM, Boris Brezillon
>> <boris.brezillon@bootlin.com> wrote:
>> > A new I3C subsystem has been added and a generic description has been
>> > created to represent the I3C bus and the devices connected on it.
>> >
>>
>> > +I2C devices
>> > +===========
>> > +
>> > +Each I2C device connected to the bus should be described in a subnode. All
>> > +properties described in Documentation/devicetree/bindings/i2c/i2c.txt are
>> > +valid here, but several new properties have been added.
>> > +
>> > +New constraint on existing properties:
>> > +--------------------------------------
>> ...
>> > +  + third cell: should be 0
>>
>> What for? Just to be future-proof, or do you have something specific
>> in mind here?
>
> Even if I2C devices only need 1 cell, I3C devices need 3 (1 for the
> static address and 2 for the PID). Since both type of devices are
> described under the same bus node, and we can have different
> #address-cells, I had to put 0-padding in the last cell.

Ok, got it.

    Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html