This series adds an algorithm for an I2C master physically located on an FSI
slave device. The I2C master has multiple ports, each of which may be connected
to an I2C slave. Access to the I2C master registers is achieved over FSI bus.
Due to the multi-port nature of the I2C master, the driver instantiates a new
I2C adapter for each port connected to a slave. The connected ports should be
defined in the device tree under the I2C master device.
Changes since v9
- Switch the status masks to use a combination of bits rather than directly
coding the value
- Remove the interrupt mask definition as it was unused
- Fixed return value of master_xfer function (return number of xfrs, not 0)
Changes since v8
- Drop unecessary else statements
- Use i++ instead of ++i
- Use kzalloc/kfree instead of devm_kzalloc/devm_kfree for port structure
- Drop the list_empty check in remove
Changes since v7
- Fix grammer in Kconfig (a -> an)
- Change I2C registers to use BIT and GENMASK
- Remove custom macros and use FIELD_PREP and FIELD_GET
- Fix a few unecessary initializations and "return rc" that are always zero
- Clean up the read/write fifo functions a bit
- Few other clean-up items
Changes since v6
- Remove spinlock for reset functionality; it's unecessary and doesn't work
with the latest FSI core.
- Use a mutex instead of a semaphore, and don't wait for timeout to get the
lock.
- Use usleeps instead of schedule_timeout; it's not worth the overhead when
the wait should be very short in between sending the command and receiving
the response.
Changes since v5
- Fix reset functionality and do a reset after every transfer failure
Eddie James (7):
dt-bindings: i2c: Add FSI-attached I2C master dt binding documentation
i2c: Add FSI-attached I2C master algorithm
i2c: fsi: Add port structures
i2c: fsi: Add abort and hardware reset procedures
i2c: fsi: Add transfer implementation
i2c: fsi: Add I2C master locking
i2c: fsi: Add bus recovery
Documentation/devicetree/bindings/i2c/i2c-fsi.txt | 40 ++
drivers/i2c/busses/Kconfig | 11 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-fsi.c | 727 ++++++++++++++++++++++
4 files changed, 779 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-fsi.txt
create mode 100644 drivers/i2c/busses/i2c-fsi.c
--
1.8.3.1
Bus recovery should reset the engine and force clock the bus 9 times
to recover most situations.
Signed-off-by: Eddie James <[email protected]>
---
drivers/i2c/busses/i2c-fsi.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index d6cab4b..940b198 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -611,6 +611,24 @@ static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
I2C_FUNC_SMBUS_BLOCK_DATA;
}
+static int fsi_i2c_recover_bus(struct i2c_adapter *adap)
+{
+ int rc;
+ struct fsi_i2c_port *port = adap->algo_data;
+ struct fsi_i2c_master *master = port->master;
+
+ mutex_lock(&master->lock);
+
+ rc = fsi_i2c_reset(master, port->port);
+
+ mutex_unlock(&master->lock);
+ return rc;
+}
+
+static struct i2c_bus_recovery_info fsi_i2c_bus_recovery_info = {
+ .recover_bus = fsi_i2c_recover_bus,
+};
+
static const struct i2c_algorithm fsi_i2c_algorithm = {
.master_xfer = fsi_i2c_xfer,
.functionality = fsi_i2c_functionality,
@@ -653,6 +671,7 @@ static int fsi_i2c_probe(struct device *dev)
port->adapter.dev.of_node = np;
port->adapter.dev.parent = dev;
port->adapter.algo = &fsi_i2c_algorithm;
+ port->adapter.bus_recovery_info = &fsi_i2c_bus_recovery_info;
port->adapter.algo_data = port;
snprintf(port->adapter.name, sizeof(port->adapter.name),
--
1.8.3.1
Execute I2C transfers from the FSI-attached I2C master. Use polling
instead of interrupts as we have no hardware IRQ over FSI.
Signed-off-by: Eddie James <[email protected]>
---
drivers/i2c/busses/i2c-fsi.c | 195 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 193 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index 4611a0b..8c0a6cb 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -155,6 +155,7 @@ struct fsi_i2c_port {
struct i2c_adapter adapter;
struct fsi_i2c_master *master;
u16 port;
+ u16 xfrd;
};
static int fsi_i2c_read_reg(struct fsi_device *fsi, unsigned int reg,
@@ -230,6 +231,99 @@ static int fsi_i2c_set_port(struct fsi_i2c_port *port)
return fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
}
+static int fsi_i2c_start(struct fsi_i2c_port *port, struct i2c_msg *msg,
+ bool stop)
+{
+ struct fsi_i2c_master *i2c = port->master;
+ u32 cmd = I2C_CMD_WITH_START | I2C_CMD_WITH_ADDR;
+
+ port->xfrd = 0;
+
+ if (msg->flags & I2C_M_RD)
+ cmd |= I2C_CMD_READ;
+
+ if (stop || msg->flags & I2C_M_STOP)
+ cmd |= I2C_CMD_WITH_STOP;
+
+ cmd |= FIELD_PREP(I2C_CMD_ADDR, msg->addr >> 1);
+ cmd |= FIELD_PREP(I2C_CMD_LEN, msg->len);
+
+ return fsi_i2c_write_reg(i2c->fsi, I2C_FSI_CMD, &cmd);
+}
+
+static int fsi_i2c_get_op_bytes(int op_bytes)
+{
+ /* fsi is limited to max 4 byte aligned ops */
+ if (op_bytes > 4)
+ return 4;
+ else if (op_bytes == 3)
+ return 2;
+ return op_bytes;
+}
+
+static int fsi_i2c_write_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
+ u8 fifo_count)
+{
+ int write;
+ int rc;
+ struct fsi_i2c_master *i2c = port->master;
+ int bytes_to_write = i2c->fifo_size - fifo_count;
+ int bytes_remaining = msg->len - port->xfrd;
+
+ bytes_to_write = min(bytes_to_write, bytes_remaining);
+
+ while (bytes_to_write) {
+ write = fsi_i2c_get_op_bytes(bytes_to_write);
+
+ rc = fsi_device_write(i2c->fsi, I2C_FSI_FIFO,
+ &msg->buf[port->xfrd], write);
+ if (rc)
+ return rc;
+
+ port->xfrd += write;
+ bytes_to_write -= write;
+ }
+
+ return 0;
+}
+
+static int fsi_i2c_read_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
+ u8 fifo_count)
+{
+ int read;
+ int rc;
+ struct fsi_i2c_master *i2c = port->master;
+ int bytes_to_read;
+ int xfr_remaining = msg->len - port->xfrd;
+ u32 dummy;
+
+ bytes_to_read = min_t(int, fifo_count, xfr_remaining);
+
+ while (bytes_to_read) {
+ read = fsi_i2c_get_op_bytes(bytes_to_read);
+
+ if (xfr_remaining) {
+ rc = fsi_device_read(i2c->fsi, I2C_FSI_FIFO,
+ &msg->buf[port->xfrd], read);
+ if (rc)
+ return rc;
+
+ port->xfrd += read;
+ xfr_remaining -= read;
+ } else {
+ /* no more buffer but data in fifo, need to clear it */
+ rc = fsi_device_read(i2c->fsi, I2C_FSI_FIFO, &dummy,
+ read);
+ if (rc)
+ return rc;
+ }
+
+ bytes_to_read -= read;
+ }
+
+ return 0;
+}
+
static int fsi_i2c_reset_bus(struct fsi_i2c_master *i2c)
{
int i, rc;
@@ -393,17 +487,114 @@ static int fsi_i2c_abort(struct fsi_i2c_port *port, u32 status)
return -ETIMEDOUT;
}
+static int fsi_i2c_handle_status(struct fsi_i2c_port *port,
+ struct i2c_msg *msg, u32 status)
+{
+ int rc;
+ u8 fifo_count;
+
+ if (status & I2C_STAT_ERR) {
+ rc = fsi_i2c_abort(port, status);
+ if (rc)
+ return rc;
+
+ if (status & I2C_STAT_INV_CMD)
+ return -EINVAL;
+
+ if (status & (I2C_STAT_PARITY | I2C_STAT_BE_OVERRUN |
+ I2C_STAT_BE_ACCESS))
+ return -EPROTO;
+
+ if (status & I2C_STAT_NACK)
+ return -ENXIO;
+
+ if (status & I2C_STAT_LOST_ARB)
+ return -EAGAIN;
+
+ if (status & I2C_STAT_STOP_ERR)
+ return -EBADMSG;
+
+ return -EIO;
+ }
+
+ if (status & I2C_STAT_DAT_REQ) {
+ fifo_count = FIELD_GET(I2C_STAT_FIFO_COUNT, status);
+
+ if (msg->flags & I2C_M_RD)
+ return fsi_i2c_read_fifo(port, msg, fifo_count);
+
+ return fsi_i2c_write_fifo(port, msg, fifo_count);
+ }
+
+ if (status & I2C_STAT_CMD_COMP) {
+ if (port->xfrd < msg->len)
+ return -ENODATA;
+
+ return msg->len;
+ }
+
+ return 0;
+}
+
+static int fsi_i2c_wait(struct fsi_i2c_port *port, struct i2c_msg *msg,
+ unsigned long timeout)
+{
+ u32 status = 0;
+ int rc;
+ unsigned long start = jiffies;
+
+ do {
+ rc = fsi_i2c_read_reg(port->master->fsi, I2C_FSI_STAT,
+ &status);
+ if (rc)
+ return rc;
+
+ if (status & I2C_STAT_ANY_RESP) {
+ rc = fsi_i2c_handle_status(port, msg, status);
+ if (rc < 0)
+ return rc;
+
+ /* cmd complete and all data xfrd */
+ if (rc == msg->len)
+ return 0;
+
+ /* need to xfr more data, but maybe don't need wait */
+ continue;
+ }
+
+ usleep_range(I2C_CMD_SLEEP_MIN_US, I2C_CMD_SLEEP_MAX_US);
+ } while (time_after(start + timeout, jiffies));
+
+ return -ETIMEDOUT;
+}
+
static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
int num)
{
- int rc;
+ int i, rc;
+ unsigned long start_time;
struct fsi_i2c_port *port = adap->algo_data;
+ struct i2c_msg *msg;
rc = fsi_i2c_set_port(port);
if (rc)
return rc;
- return -EOPNOTSUPP;
+ for (i = 0; i < num; i++) {
+ msg = msgs + i;
+ start_time = jiffies;
+
+ rc = fsi_i2c_start(port, msg, i == num - 1);
+ if (rc)
+ return rc;
+
+ rc = fsi_i2c_wait(port, msg,
+ adap->timeout - (jiffies - start_time));
+ if (rc)
+ return rc;
+ }
+
+ return num;
}
static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
--
1.8.3.1
Since there are many ports per master, each with it's own adapter and
chardev, we need some locking to prevent transfers from changing the
master state while other transfers are in progress.
Signed-off-by: Eddie James <[email protected]>
---
drivers/i2c/busses/i2c-fsi.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index 8c0a6cb..d6cab4b 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -21,6 +21,7 @@
#include <linux/kernel.h>
#include <linux/list.h>
#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/of.h>
#include <linux/slab.h>
@@ -148,6 +149,7 @@ struct fsi_i2c_master {
struct fsi_device *fsi;
u8 fifo_size;
struct list_head ports;
+ struct mutex lock;
};
struct fsi_i2c_port {
@@ -574,11 +576,14 @@ static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
int i, rc;
unsigned long start_time;
struct fsi_i2c_port *port = adap->algo_data;
+ struct fsi_i2c_master *master = port->master;
struct i2c_msg *msg;
+ mutex_lock(&master->lock);
+
rc = fsi_i2c_set_port(port);
if (rc)
- return rc;
+ goto unlock;
for (i = 0; i < num; i++) {
msg = msgs + i;
@@ -586,15 +591,17 @@ static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
rc = fsi_i2c_start(port, msg, i == num - 1);
if (rc)
- return rc;
+ goto unlock;
rc = fsi_i2c_wait(port, msg,
adap->timeout - (jiffies - start_time));
if (rc)
- return rc;
+ goto unlock;
}
- return num;
+unlock:
+ mutex_unlock(&master->lock);
+ return rc ? : num;
}
static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
@@ -621,6 +628,7 @@ static int fsi_i2c_probe(struct device *dev)
if (!i2c)
return -ENOMEM;
+ mutex_init(&i2c->lock);
i2c->fsi = to_fsi_dev(dev);
INIT_LIST_HEAD(&i2c->ports);
--
1.8.3.1
Add abort procedure for failed transfers. Add engine and bus reset
procedures to recover from as many faults as possible.
Signed-off-by: Eddie James <[email protected]>
---
drivers/i2c/busses/i2c-fsi.c | 179 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 179 insertions(+)
diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index 695818f..4611a0b 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -12,10 +12,12 @@
#include <linux/bitfield.h>
#include <linux/bitops.h>
+#include <linux/delay.h>
#include <linux/device.h>
#include <linux/errno.h>
#include <linux/fsi.h>
#include <linux/i2c.h>
+#include <linux/jiffies.h>
#include <linux/kernel.h>
#include <linux/list.h>
#include <linux/module.h>
@@ -128,6 +130,20 @@
#define I2C_ESTAT_SELF_BUSY BIT(6)
#define I2C_ESTAT_VERSION GENMASK(4, 0)
+/* port busy register */
+#define I2C_PORT_BUSY_RESET BIT(31)
+
+/* wait for command complete or data request */
+#define I2C_CMD_SLEEP_MAX_US 500
+#define I2C_CMD_SLEEP_MIN_US 50
+
+/* wait after reset; choose time from legacy driver */
+#define I2C_RESET_SLEEP_MAX_US 2000
+#define I2C_RESET_SLEEP_MIN_US 1000
+
+/* choose timeout length from legacy driver; it's well tested */
+#define I2C_ABORT_TIMEOUT msecs_to_jiffies(100)
+
struct fsi_i2c_master {
struct fsi_device *fsi;
u8 fifo_size;
@@ -214,6 +230,169 @@ static int fsi_i2c_set_port(struct fsi_i2c_port *port)
return fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
}
+static int fsi_i2c_reset_bus(struct fsi_i2c_master *i2c)
+{
+ int i, rc;
+ u32 mode, stat, ext, dummy = 0;
+
+ rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+ if (rc)
+ return rc;
+
+ mode |= I2C_MODE_DIAG;
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+ if (rc)
+ return rc;
+
+ for (i = 0; i < 9; i++) {
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_SCL, &dummy);
+ if (rc)
+ return rc;
+
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_SET_SCL, &dummy);
+ if (rc)
+ return rc;
+ }
+
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_SCL, &dummy);
+ if (rc)
+ return rc;
+
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_SDA, &dummy);
+ if (rc)
+ return rc;
+
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_SET_SCL, &dummy);
+ if (rc)
+ return rc;
+
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_SET_SDA, &dummy);
+ if (rc)
+ return rc;
+
+ mode &= ~I2C_MODE_DIAG;
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+ if (rc)
+ return rc;
+
+ rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_STAT, &stat);
+ if (rc)
+ return rc;
+
+ /* check for hardware fault */
+ if (!(stat & I2C_STAT_SCL_IN) || !(stat & I2C_STAT_SDA_IN)) {
+ rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_ESTAT, &ext);
+ if (rc)
+ return rc;
+
+ dev_err(&i2c->fsi->dev, "bus stuck status[%08X] ext[%08X]\n",
+ stat, ext);
+ }
+
+ return 0;
+}
+
+static int fsi_i2c_reset(struct fsi_i2c_master *i2c, u16 port)
+{
+ int rc;
+ u32 mode, stat, dummy = 0;
+
+ /* reset engine */
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_I2C, &dummy);
+ if (rc)
+ return rc;
+
+ /* re-init engine */
+ rc = fsi_i2c_dev_init(i2c);
+ if (rc)
+ return rc;
+
+ rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+ if (rc)
+ return rc;
+
+ /* set port; default after reset is 0 */
+ if (port) {
+ mode &= ~I2C_MODE_PORT;
+ mode |= FIELD_PREP(I2C_MODE_PORT, port);
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+ if (rc)
+ return rc;
+ }
+
+ /* reset busy register; hw workaround */
+ dummy = I2C_PORT_BUSY_RESET;
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_PORT_BUSY, &dummy);
+ if (rc)
+ return rc;
+
+ /* force bus reset */
+ rc = fsi_i2c_reset_bus(i2c);
+ if (rc)
+ return rc;
+
+ /* reset errors */
+ dummy = 0;
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_ERR, &dummy);
+ if (rc)
+ return rc;
+
+ /* wait for command complete */
+ usleep_range(I2C_RESET_SLEEP_MIN_US, I2C_RESET_SLEEP_MAX_US);
+
+ rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_STAT, &stat);
+ if (rc)
+ return rc;
+
+ if (stat & I2C_STAT_CMD_COMP)
+ return rc;
+
+ /* failed to get command complete; reset engine again */
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_I2C, &dummy);
+ if (rc)
+ return rc;
+
+ /* re-init engine again */
+ return fsi_i2c_dev_init(i2c);
+}
+
+static int fsi_i2c_abort(struct fsi_i2c_port *port, u32 status)
+{
+ int rc;
+ unsigned long start;
+ u32 cmd = I2C_CMD_WITH_STOP;
+ struct fsi_device *fsi = port->master->fsi;
+
+ rc = fsi_i2c_reset(port->master, port->port);
+ if (rc)
+ return rc;
+
+ /* skip final stop command for these errors */
+ if (status & (I2C_STAT_PARITY | I2C_STAT_LOST_ARB | I2C_STAT_STOP_ERR))
+ return 0;
+
+ /* write stop command */
+ rc = fsi_i2c_write_reg(fsi, I2C_FSI_CMD, &cmd);
+ if (rc)
+ return rc;
+
+ /* wait until we see command complete in the master */
+ start = jiffies;
+
+ do {
+ rc = fsi_i2c_read_reg(fsi, I2C_FSI_STAT, &status);
+ if (rc)
+ return rc;
+
+ if (status & I2C_STAT_CMD_COMP)
+ return 0;
+
+ usleep_range(I2C_CMD_SLEEP_MIN_US, I2C_CMD_SLEEP_MAX_US);
+ } while (time_after(start + I2C_ABORT_TIMEOUT, jiffies));
+
+ return -ETIMEDOUT;
+}
+
static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
int num)
{
--
1.8.3.1
Add register definitions for FSI-attached I2C master and functions to
access those registers over FSI. Add an FSI driver so that our I2C bus
is probed up during an FSI scan.
Signed-off-by: Eddie James <[email protected]>
---
drivers/i2c/busses/Kconfig | 11 ++
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-fsi.c | 240 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 252 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-fsi.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 4f8df2e..cddd159 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1330,4 +1330,15 @@ config I2C_ZX2967
This driver can also be built as a module. If so, the module will be
called i2c-zx2967.
+config I2C_FSI
+ tristate "FSI I2C driver"
+ depends on FSI
+ help
+ Driver for FSI bus attached I2C masters. These are I2C masters that
+ are connected to the system over an FSI bus, instead of the more
+ common PCI or MMIO interface.
+
+ This driver can also be built as a module. If so, the module will be
+ called as i2c-fsi.
+
endmenu
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 5a86914..4909fd6 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -137,5 +137,6 @@ obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o
obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o
obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
+obj-$(CONFIG_I2C_FSI) += i2c-fsi.o
ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
new file mode 100644
index 0000000..694bbb4
--- /dev/null
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -0,0 +1,240 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * FSI-attached I2C master algorithm
+ *
+ * Copyright 2018 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/fsi.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#define FSI_ENGID_I2C 0x7
+
+#define I2C_DEFAULT_CLK_DIV 6
+
+/* i2c registers */
+#define I2C_FSI_FIFO 0x00
+#define I2C_FSI_CMD 0x04
+#define I2C_FSI_MODE 0x08
+#define I2C_FSI_WATER_MARK 0x0C
+#define I2C_FSI_INT_MASK 0x10
+#define I2C_FSI_INT_COND 0x14
+#define I2C_FSI_OR_INT_MASK 0x14
+#define I2C_FSI_INTS 0x18
+#define I2C_FSI_AND_INT_MASK 0x18
+#define I2C_FSI_STAT 0x1C
+#define I2C_FSI_RESET_I2C 0x1C
+#define I2C_FSI_ESTAT 0x20
+#define I2C_FSI_RESET_ERR 0x20
+#define I2C_FSI_RESID_LEN 0x24
+#define I2C_FSI_SET_SCL 0x24
+#define I2C_FSI_PORT_BUSY 0x28
+#define I2C_FSI_RESET_SCL 0x2C
+#define I2C_FSI_SET_SDA 0x30
+#define I2C_FSI_RESET_SDA 0x34
+
+/* cmd register */
+#define I2C_CMD_WITH_START BIT(31)
+#define I2C_CMD_WITH_ADDR BIT(30)
+#define I2C_CMD_RD_CONT BIT(29)
+#define I2C_CMD_WITH_STOP BIT(28)
+#define I2C_CMD_FORCELAUNCH BIT(27)
+#define I2C_CMD_ADDR GENMASK(23, 17)
+#define I2C_CMD_READ BIT(16)
+#define I2C_CMD_LEN GENMASK(15, 0)
+
+/* mode register */
+#define I2C_MODE_CLKDIV GENMASK(31, 16)
+#define I2C_MODE_PORT GENMASK(15, 10)
+#define I2C_MODE_ENHANCED BIT(3)
+#define I2C_MODE_DIAG BIT(2)
+#define I2C_MODE_PACE_ALLOW BIT(1)
+#define I2C_MODE_WRAP BIT(0)
+
+/* watermark register */
+#define I2C_WATERMARK_HI GENMASK(15, 12)
+#define I2C_WATERMARK_LO GENMASK(7, 4)
+
+#define I2C_FIFO_HI_LVL 4
+#define I2C_FIFO_LO_LVL 4
+
+/* interrupt register */
+#define I2C_INT_INV_CMD BIT(15)
+#define I2C_INT_PARITY BIT(14)
+#define I2C_INT_BE_OVERRUN BIT(13)
+#define I2C_INT_BE_ACCESS BIT(12)
+#define I2C_INT_LOST_ARB BIT(11)
+#define I2C_INT_NACK BIT(10)
+#define I2C_INT_DAT_REQ BIT(9)
+#define I2C_INT_CMD_COMP BIT(8)
+#define I2C_INT_STOP_ERR BIT(7)
+#define I2C_INT_BUSY BIT(6)
+#define I2C_INT_IDLE BIT(5)
+
+/* status register */
+#define I2C_STAT_INV_CMD BIT(31)
+#define I2C_STAT_PARITY BIT(30)
+#define I2C_STAT_BE_OVERRUN BIT(29)
+#define I2C_STAT_BE_ACCESS BIT(28)
+#define I2C_STAT_LOST_ARB BIT(27)
+#define I2C_STAT_NACK BIT(26)
+#define I2C_STAT_DAT_REQ BIT(25)
+#define I2C_STAT_CMD_COMP BIT(24)
+#define I2C_STAT_STOP_ERR BIT(23)
+#define I2C_STAT_MAX_PORT GENMASK(19, 16)
+#define I2C_STAT_ANY_INT BIT(15)
+#define I2C_STAT_SCL_IN BIT(11)
+#define I2C_STAT_SDA_IN BIT(10)
+#define I2C_STAT_PORT_BUSY BIT(9)
+#define I2C_STAT_SELF_BUSY BIT(8)
+#define I2C_STAT_FIFO_COUNT GENMASK(7, 0)
+
+#define I2C_STAT_ERR (I2C_STAT_INV_CMD | \
+ I2C_STAT_PARITY | \
+ I2C_STAT_BE_OVERRUN | \
+ I2C_STAT_BE_ACCESS | \
+ I2C_STAT_LOST_ARB | \
+ I2C_STAT_NACK | \
+ I2C_STAT_STOP_ERR)
+#define I2C_STAT_ANY_RESP (I2C_STAT_ERR | \
+ I2C_STAT_DAT_REQ | \
+ I2C_STAT_CMD_COMP)
+
+/* extended status register */
+#define I2C_ESTAT_FIFO_SZ GENMASK(31, 24)
+#define I2C_ESTAT_SCL_IN_SY BIT(15)
+#define I2C_ESTAT_SDA_IN_SY BIT(14)
+#define I2C_ESTAT_S_SCL BIT(13)
+#define I2C_ESTAT_S_SDA BIT(12)
+#define I2C_ESTAT_M_SCL BIT(11)
+#define I2C_ESTAT_M_SDA BIT(10)
+#define I2C_ESTAT_HI_WATER BIT(9)
+#define I2C_ESTAT_LO_WATER BIT(8)
+#define I2C_ESTAT_PORT_BUSY BIT(7)
+#define I2C_ESTAT_SELF_BUSY BIT(6)
+#define I2C_ESTAT_VERSION GENMASK(4, 0)
+
+struct fsi_i2c_master {
+ struct fsi_device *fsi;
+ u8 fifo_size;
+};
+
+static int fsi_i2c_read_reg(struct fsi_device *fsi, unsigned int reg,
+ u32 *data)
+{
+ int rc;
+ __be32 data_be;
+
+ rc = fsi_device_read(fsi, reg, &data_be, sizeof(data_be));
+ if (rc)
+ return rc;
+
+ *data = be32_to_cpu(data_be);
+
+ return 0;
+}
+
+static int fsi_i2c_write_reg(struct fsi_device *fsi, unsigned int reg,
+ u32 *data)
+{
+ __be32 data_be = cpu_to_be32p(data);
+
+ return fsi_device_write(fsi, reg, &data_be, sizeof(data_be));
+}
+
+static int fsi_i2c_dev_init(struct fsi_i2c_master *i2c)
+{
+ int rc;
+ u32 mode = I2C_MODE_ENHANCED, extended_status, watermark;
+ u32 interrupt = 0;
+
+ /* since we use polling, disable interrupts */
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_INT_MASK, &interrupt);
+ if (rc)
+ return rc;
+
+ mode |= FIELD_PREP(I2C_MODE_CLKDIV, I2C_DEFAULT_CLK_DIV);
+ rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+ if (rc)
+ return rc;
+
+ rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_ESTAT, &extended_status);
+ if (rc)
+ return rc;
+
+ i2c->fifo_size = FIELD_GET(I2C_ESTAT_FIFO_SZ, extended_status);
+ watermark = FIELD_PREP(I2C_WATERMARK_HI,
+ i2c->fifo_size - I2C_FIFO_HI_LVL);
+ watermark |= FIELD_PREP(I2C_WATERMARK_LO, I2C_FIFO_LO_LVL);
+
+ return fsi_i2c_write_reg(i2c->fsi, I2C_FSI_WATER_MARK, &watermark);
+}
+
+static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+ int num)
+{
+ return -EOPNOTSUPP;
+}
+
+static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_PROTOCOL_MANGLING |
+ I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL |
+ I2C_FUNC_SMBUS_BLOCK_DATA;
+}
+
+static const struct i2c_algorithm fsi_i2c_algorithm = {
+ .master_xfer = fsi_i2c_xfer,
+ .functionality = fsi_i2c_functionality,
+};
+
+static int fsi_i2c_probe(struct device *dev)
+{
+ struct fsi_i2c_master *i2c;
+ int rc;
+
+ i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
+ if (!i2c)
+ return -ENOMEM;
+
+ i2c->fsi = to_fsi_dev(dev);
+
+ rc = fsi_i2c_dev_init(i2c);
+ if (rc)
+ return rc;
+
+ dev_set_drvdata(dev, i2c);
+
+ return 0;
+}
+
+static const struct fsi_device_id fsi_i2c_ids[] = {
+ { FSI_ENGID_I2C, FSI_VERSION_ANY },
+ { }
+};
+
+static struct fsi_driver fsi_i2c_driver = {
+ .id_table = fsi_i2c_ids,
+ .drv = {
+ .name = "i2c-fsi",
+ .bus = &fsi_bus_type,
+ .probe = fsi_i2c_probe,
+ },
+};
+
+module_fsi_driver(fsi_i2c_driver);
+
+MODULE_AUTHOR("Eddie James <[email protected]>");
+MODULE_DESCRIPTION("FSI attached I2C master");
+MODULE_LICENSE("GPL");
--
1.8.3.1
Document the bindings.
Signed-off-by: Eddie James <[email protected]>>
Acked-by: Rob Herring <[email protected]>
---
Documentation/devicetree/bindings/i2c/i2c-fsi.txt | 40 +++++++++++++++++++++++
1 file changed, 40 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-fsi.txt
diff --git a/Documentation/devicetree/bindings/i2c/i2c-fsi.txt b/Documentation/devicetree/bindings/i2c/i2c-fsi.txt
new file mode 100644
index 0000000..b1be2ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-fsi.txt
@@ -0,0 +1,40 @@
+Device-tree bindings for FSI-attached I2C master and busses
+-----------------------------------------------------------
+
+Required properties:
+ - compatible = "ibm,i2c-fsi";
+ - reg = < address size >; : The FSI CFAM address and address
+ space size.
+ - #address-cells = <1>; : Number of address cells in child
+ nodes.
+ - #size-cells = <0>; : Number of size cells in child nodes.
+ - child nodes : Nodes to describe busses off the I2C
+ master.
+
+Child node required properties:
+ - reg = < port number > : The port number on the I2C master.
+
+Child node optional properties:
+ - child nodes : Nodes to describe devices on the I2C
+ bus.
+
+Examples:
+
+ i2c@1800 {
+ compatible = "ibm,i2c-fsi";
+ reg = < 0x1800 0x400 >;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ i2c-bus@0 {
+ reg = <0>;
+ };
+
+ i2c-bus@1 {
+ reg = <1>;
+
+ eeprom@50 {
+ compatible = "vendor,dev-name";
+ };
+ };
+ };
--
1.8.3.1
Add and initialize I2C adapters for each port on the FSI-attached I2C
master. Ports for each master are defined in the devicetree.
Signed-off-by: Eddie James <[email protected]>
---
drivers/i2c/busses/i2c-fsi.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 90 insertions(+)
diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index 694bbb4..695818f 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -17,7 +17,10 @@
#include <linux/fsi.h>
#include <linux/i2c.h>
#include <linux/kernel.h>
+#include <linux/list.h>
#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
#define FSI_ENGID_I2C 0x7
@@ -128,6 +131,14 @@
struct fsi_i2c_master {
struct fsi_device *fsi;
u8 fifo_size;
+ struct list_head ports;
+};
+
+struct fsi_i2c_port {
+ struct list_head list;
+ struct i2c_adapter adapter;
+ struct fsi_i2c_master *master;
+ u16 port;
};
static int fsi_i2c_read_reg(struct fsi_device *fsi, unsigned int reg,
@@ -181,9 +192,38 @@ static int fsi_i2c_dev_init(struct fsi_i2c_master *i2c)
return fsi_i2c_write_reg(i2c->fsi, I2C_FSI_WATER_MARK, &watermark);
}
+static int fsi_i2c_set_port(struct fsi_i2c_port *port)
+{
+ int rc;
+ struct fsi_device *fsi = port->master->fsi;
+ u32 mode, dummy = 0;
+
+ rc = fsi_i2c_read_reg(fsi, I2C_FSI_MODE, &mode);
+ if (rc)
+ return rc;
+
+ if (FIELD_GET(I2C_MODE_PORT, mode) == port->port)
+ return 0;
+
+ mode = (mode & ~I2C_MODE_PORT) | FIELD_PREP(I2C_MODE_PORT, port->port);
+ rc = fsi_i2c_write_reg(fsi, I2C_FSI_MODE, &mode);
+ if (rc)
+ return rc;
+
+ /* reset engine when port is changed */
+ return fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
+}
+
static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
int num)
{
+ int rc;
+ struct fsi_i2c_port *port = adap->algo_data;
+
+ rc = fsi_i2c_set_port(port);
+ if (rc)
+ return rc;
+
return -EOPNOTSUPP;
}
@@ -202,23 +242,72 @@ static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
static int fsi_i2c_probe(struct device *dev)
{
struct fsi_i2c_master *i2c;
+ struct fsi_i2c_port *port;
+ struct device_node *np;
int rc;
+ u32 port_no;
i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
if (!i2c)
return -ENOMEM;
i2c->fsi = to_fsi_dev(dev);
+ INIT_LIST_HEAD(&i2c->ports);
rc = fsi_i2c_dev_init(i2c);
if (rc)
return rc;
+ /* Add adapter for each i2c port of the master. */
+ for_each_available_child_of_node(dev->of_node, np) {
+ rc = of_property_read_u32(np, "reg", &port_no);
+ if (rc || port_no > USHRT_MAX)
+ continue;
+
+ port = kzalloc(sizeof(*port), GFP_KERNEL);
+ if (!port)
+ break;
+
+ port->master = i2c;
+ port->port = port_no;
+
+ port->adapter.owner = THIS_MODULE;
+ port->adapter.dev.of_node = np;
+ port->adapter.dev.parent = dev;
+ port->adapter.algo = &fsi_i2c_algorithm;
+ port->adapter.algo_data = port;
+
+ snprintf(port->adapter.name, sizeof(port->adapter.name),
+ "i2c_bus-%u", port_no);
+
+ rc = i2c_add_adapter(&port->adapter);
+ if (rc < 0) {
+ dev_err(dev, "Failed to register adapter: %d\n", rc);
+ kfree(port);
+ continue;
+ }
+
+ list_add(&port->list, &i2c->ports);
+ }
+
dev_set_drvdata(dev, i2c);
return 0;
}
+static int fsi_i2c_remove(struct device *dev)
+{
+ struct fsi_i2c_master *i2c = dev_get_drvdata(dev);
+ struct fsi_i2c_port *port;
+
+ list_for_each_entry(port, &i2c->ports, list) {
+ i2c_del_adapter(&port->adapter);
+ kfree(port);
+ }
+
+ return 0;
+}
+
static const struct fsi_device_id fsi_i2c_ids[] = {
{ FSI_ENGID_I2C, FSI_VERSION_ANY },
{ }
@@ -230,6 +319,7 @@ static int fsi_i2c_probe(struct device *dev)
.name = "i2c-fsi",
.bus = &fsi_bus_type,
.probe = fsi_i2c_probe,
+ .remove = fsi_i2c_remove,
},
};
--
1.8.3.1
On Wed, Jun 13, 2018 at 10:36 PM, Eddie James
<[email protected]> wrote:
> This series adds an algorithm for an I2C master physically located on an FSI
> slave device. The I2C master has multiple ports, each of which may be connected
> to an I2C slave. Access to the I2C master registers is achieved over FSI bus.
>
> Due to the multi-port nature of the I2C master, the driver instantiates a new
> I2C adapter for each port connected to a slave. The connected ports should be
> defined in the device tree under the I2C master device.
>
You missed to include tag I gave
Reviewed-by: Andy Shevchenko <[email protected]>
be careful next time.
> Changes since v9
> - Switch the status masks to use a combination of bits rather than directly
> coding the value
> - Remove the interrupt mask definition as it was unused
> - Fixed return value of master_xfer function (return number of xfrs, not 0)
>
> Changes since v8
> - Drop unecessary else statements
> - Use i++ instead of ++i
> - Use kzalloc/kfree instead of devm_kzalloc/devm_kfree for port structure
> - Drop the list_empty check in remove
>
> Changes since v7
> - Fix grammer in Kconfig (a -> an)
> - Change I2C registers to use BIT and GENMASK
> - Remove custom macros and use FIELD_PREP and FIELD_GET
> - Fix a few unecessary initializations and "return rc" that are always zero
> - Clean up the read/write fifo functions a bit
> - Few other clean-up items
>
> Changes since v6
> - Remove spinlock for reset functionality; it's unecessary and doesn't work
> with the latest FSI core.
> - Use a mutex instead of a semaphore, and don't wait for timeout to get the
> lock.
> - Use usleeps instead of schedule_timeout; it's not worth the overhead when
> the wait should be very short in between sending the command and receiving
> the response.
>
> Changes since v5
> - Fix reset functionality and do a reset after every transfer failure
>
> Eddie James (7):
> dt-bindings: i2c: Add FSI-attached I2C master dt binding documentation
> i2c: Add FSI-attached I2C master algorithm
> i2c: fsi: Add port structures
> i2c: fsi: Add abort and hardware reset procedures
> i2c: fsi: Add transfer implementation
> i2c: fsi: Add I2C master locking
> i2c: fsi: Add bus recovery
>
> Documentation/devicetree/bindings/i2c/i2c-fsi.txt | 40 ++
> drivers/i2c/busses/Kconfig | 11 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-fsi.c | 727 ++++++++++++++++++++++
> 4 files changed, 779 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-fsi.txt
> create mode 100644 drivers/i2c/busses/i2c-fsi.c
>
> --
> 1.8.3.1
>
--
With Best Regards,
Andy Shevchenko
On 14 June 2018 at 05:06, Eddie James <[email protected]> wrote:
> This series adds an algorithm for an I2C master physically located on an FSI
> slave device. The I2C master has multiple ports, each of which may be connected
> to an I2C slave. Access to the I2C master registers is achieved over FSI bus.
>
> Due to the multi-port nature of the I2C master, the driver instantiates a new
> I2C adapter for each port connected to a slave. The connected ports should be
> defined in the device tree under the I2C master device.
Looks good Eddie. I had a read over what you have, and I would be
happy to see this merged in 4.19.
Reviewed-By: Joel Stanley <[email protected]>
Also, I have merged this into the 4.17 based openbmc tree for testing
while we wait for it to be merged upstream.
Cheers,
Joel
On Wed, 2018-06-13 at 14:36 -0500, Eddie James wrote:
> }
>
> +static int fsi_i2c_remove(struct device *dev)
> +{
> + struct fsi_i2c_master *i2c = dev_get_drvdata(dev);
> + struct fsi_i2c_port *port;
> +
> + list_for_each_entry(port, &i2c->ports, list) {
> + i2c_del_adapter(&port->adapter);
> + kfree(port);
> + }
> +
> + return 0;
> +}
> +
This is a use-after-free, the list linkage of the freed port is used to
get to the next one. With memory poisoning, kbooom !
You can fold that in:
From f9d9092160897e7308f6990067a03e937339537f Mon Sep 17 00:00:00 2001
From: Benjamin Herrenschmidt <[email protected]>
Date: Wed, 20 Jun 2018 13:27:32 +1000
Subject: [PATCH] i2c: fsi: Fix use after free
Signed-off-by: Benjamin Herrenschmidt <[email protected]>
---
drivers/i2c/busses/i2c-fsi.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index 713959b44403..ff69ab6aa79a 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -696,9 +696,10 @@ static int fsi_i2c_probe(struct device *dev)
static int fsi_i2c_remove(struct device *dev)
{
struct fsi_i2c_master *i2c = dev_get_drvdata(dev);
- struct fsi_i2c_port *port;
+ struct fsi_i2c_port *port, *tmp;
- list_for_each_entry(port, &i2c->ports, list) {
+ list_for_each_entry_safe(port,tmp, &i2c->ports, list) {
+ list_del(&port->list);
i2c_del_adapter(&port->adapter);
kfree(port);
}
On 20 June 2018 at 13:04, Benjamin Herrenschmidt
<[email protected]> wrote:
> On Wed, 2018-06-13 at 14:36 -0500, Eddie James wrote:
>> }
>>
>> +static int fsi_i2c_remove(struct device *dev)
>> +{
>> + struct fsi_i2c_master *i2c = dev_get_drvdata(dev);
>> + struct fsi_i2c_port *port;
>> +
>> + list_for_each_entry(port, &i2c->ports, list) {
>> + i2c_del_adapter(&port->adapter);
>> + kfree(port);
>> + }
>> +
>> + return 0;
>> +}
>> +
>
> This is a use-after-free, the list linkage of the freed port is used to
> get to the next one. With memory poisoning, kbooom !
>
> You can fold that in:
>
> From f9d9092160897e7308f6990067a03e937339537f Mon Sep 17 00:00:00 2001
> From: Benjamin Herrenschmidt <[email protected]>
> Date: Wed, 20 Jun 2018 13:27:32 +1000
> Subject: [PATCH] i2c: fsi: Fix use after free
This fixes the issue I was seeing. For the series:
Tested-by: Joel Stanley <[email protected]>
Thanks,
Joel
>
> Signed-off-by: Benjamin Herrenschmidt <[email protected]>
> ---
> drivers/i2c/busses/i2c-fsi.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
> index 713959b44403..ff69ab6aa79a 100644
> --- a/drivers/i2c/busses/i2c-fsi.c
> +++ b/drivers/i2c/busses/i2c-fsi.c
> @@ -696,9 +696,10 @@ static int fsi_i2c_probe(struct device *dev)
> static int fsi_i2c_remove(struct device *dev)
> {
> struct fsi_i2c_master *i2c = dev_get_drvdata(dev);
> - struct fsi_i2c_port *port;
> + struct fsi_i2c_port *port, *tmp;
>
> - list_for_each_entry(port, &i2c->ports, list) {
> + list_for_each_entry_safe(port,tmp, &i2c->ports, list) {
> + list_del(&port->list);
> i2c_del_adapter(&port->adapter);
> kfree(port);
> }
>
On Wed, Jun 13, 2018 at 02:36:19PM -0500, Eddie James wrote:
> Bus recovery should reset the engine and force clock the bus 9 times
> to recover most situations.
>
> Signed-off-by: Eddie James <[email protected]>
> ---
> drivers/i2c/busses/i2c-fsi.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
> index d6cab4b..940b198 100644
> --- a/drivers/i2c/busses/i2c-fsi.c
> +++ b/drivers/i2c/busses/i2c-fsi.c
> @@ -611,6 +611,24 @@ static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
> I2C_FUNC_SMBUS_BLOCK_DATA;
> }
>
> +static int fsi_i2c_recover_bus(struct i2c_adapter *adap)
> +{
> + int rc;
> + struct fsi_i2c_port *port = adap->algo_data;
> + struct fsi_i2c_master *master = port->master;
> +
> + mutex_lock(&master->lock);
> +
> + rc = fsi_i2c_reset(master, port->port);
> +
> + mutex_unlock(&master->lock);
> + return rc;
> +}
> +
> +static struct i2c_bus_recovery_info fsi_i2c_bus_recovery_info = {
> + .recover_bus = fsi_i2c_recover_bus,
> +};
> +
This all won't have any effect since you never call i2c_recover_bus
which calls back into i2c_bus_recovery_info callbacks.
On Wed, Jun 13, 2018 at 02:36:17PM -0500, Eddie James wrote:
> Execute I2C transfers from the FSI-attached I2C master. Use polling
> instead of interrupts as we have no hardware IRQ over FSI.
>
> Signed-off-by: Eddie James <[email protected]>
> ---
> drivers/i2c/busses/i2c-fsi.c | 195 ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 193 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
> index 4611a0b..8c0a6cb 100644
> --- a/drivers/i2c/busses/i2c-fsi.c
> +++ b/drivers/i2c/busses/i2c-fsi.c
> @@ -155,6 +155,7 @@ struct fsi_i2c_port {
> struct i2c_adapter adapter;
> struct fsi_i2c_master *master;
> u16 port;
> + u16 xfrd;
> };
>
> static int fsi_i2c_read_reg(struct fsi_device *fsi, unsigned int reg,
> @@ -230,6 +231,99 @@ static int fsi_i2c_set_port(struct fsi_i2c_port *port)
> return fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
> }
>
> +static int fsi_i2c_start(struct fsi_i2c_port *port, struct i2c_msg *msg,
> + bool stop)
> +{
> + struct fsi_i2c_master *i2c = port->master;
> + u32 cmd = I2C_CMD_WITH_START | I2C_CMD_WITH_ADDR;
> +
> + port->xfrd = 0;
> +
> + if (msg->flags & I2C_M_RD)
> + cmd |= I2C_CMD_READ;
Since you support MANGLING, I'd think you can easily support
I2C_M_REV_DIR_ADDR here, too?
On Wed, Jun 13, 2018 at 02:36:13PM -0500, Eddie James wrote:
> Document the bindings.
>
> Signed-off-by: Eddie James <[email protected]>>
Broken email address here. checkpatch warns about it.
On Wed, Jun 13, 2018 at 02:36:16PM -0500, Eddie James wrote:
> Add abort procedure for failed transfers. Add engine and bus reset
> procedures to recover from as many faults as possible.
I think this is a way too aggressive recovery. Your are doing the 9
pulse toggles basically on any error while this is only when the device
keeps SDA low and you want to recover from that. If SDA is not stuck
low, sending a STOP should do. Or do you have a known case where this is
not going to work?
Also, you implement the pulse toggling manually. Can't you just populate
{get|set}_{scl|sda} and use the generic routine we have in the core?
On Wed, Jun 13, 2018 at 02:36:12PM -0500, Eddie James wrote:
> This series adds an algorithm for an I2C master physically located on an FSI
> slave device. The I2C master has multiple ports, each of which may be connected
> to an I2C slave. Access to the I2C master registers is achieved over FSI bus.
>
> Due to the multi-port nature of the I2C master, the driver instantiates a new
> I2C adapter for each port connected to a slave. The connected ports should be
> defined in the device tree under the I2C master device.
Thanks for this series and your patience.
While I can see why it also helps reviewing to send it as a series of
multiple patches, I consider applying the driver itself as just one
hunk. I am not decided on this yet.
I have a few comments, especially about recovery. I replied to the
relevant patches with more detail.
Also, are you (or someone from your company) willing to maintain the
driver? Then, an addition to MAINTAINERS would be much appreciated.
Thanks,
Wolfram
On 06/25/2018 09:38 PM, Wolfram Sang wrote:
> On Wed, Jun 13, 2018 at 02:36:17PM -0500, Eddie James wrote:
>> Execute I2C transfers from the FSI-attached I2C master. Use polling
>> instead of interrupts as we have no hardware IRQ over FSI.
>>
>> Signed-off-by: Eddie James <[email protected]>
>> ---
>> drivers/i2c/busses/i2c-fsi.c | 195 ++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 193 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
>> index 4611a0b..8c0a6cb 100644
>> --- a/drivers/i2c/busses/i2c-fsi.c
>> +++ b/drivers/i2c/busses/i2c-fsi.c
>> @@ -155,6 +155,7 @@ struct fsi_i2c_port {
>> struct i2c_adapter adapter;
>> struct fsi_i2c_master *master;
>> u16 port;
>> + u16 xfrd;
>> };
>>
>> static int fsi_i2c_read_reg(struct fsi_device *fsi, unsigned int reg,
>> @@ -230,6 +231,99 @@ static int fsi_i2c_set_port(struct fsi_i2c_port *port)
>> return fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
>> }
>>
>> +static int fsi_i2c_start(struct fsi_i2c_port *port, struct i2c_msg *msg,
>> + bool stop)
>> +{
>> + struct fsi_i2c_master *i2c = port->master;
>> + u32 cmd = I2C_CMD_WITH_START | I2C_CMD_WITH_ADDR;
>> +
>> + port->xfrd = 0;
>> +
>> + if (msg->flags & I2C_M_RD)
>> + cmd |= I2C_CMD_READ;
> Since you support MANGLING, I'd think you can easily support
> I2C_M_REV_DIR_ADDR here, too?
Hm, I don't really understand the purpose of that flag. From the docs:
This toggles the Rd/Wr flag. That is, if you want to do a write, but
need to emit an Rd instead of a Wr, or vice versa, you set this
flag. For example:
S Addr Rd [A] Data [A] Data [A] ... [A] Data [A] P
I don't think our hardware supports this type of operation.
Thanks,
Eddie
>
On 06/25/2018 09:38 PM, Wolfram Sang wrote:
> On Wed, Jun 13, 2018 at 02:36:19PM -0500, Eddie James wrote:
>> Bus recovery should reset the engine and force clock the bus 9 times
>> to recover most situations.
>>
>> Signed-off-by: Eddie James <[email protected]>
>> ---
>> drivers/i2c/busses/i2c-fsi.c | 19 +++++++++++++++++++
>> 1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
>> index d6cab4b..940b198 100644
>> --- a/drivers/i2c/busses/i2c-fsi.c
>> +++ b/drivers/i2c/busses/i2c-fsi.c
>> @@ -611,6 +611,24 @@ static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
>> I2C_FUNC_SMBUS_BLOCK_DATA;
>> }
>>
>> +static int fsi_i2c_recover_bus(struct i2c_adapter *adap)
>> +{
>> + int rc;
>> + struct fsi_i2c_port *port = adap->algo_data;
>> + struct fsi_i2c_master *master = port->master;
>> +
>> + mutex_lock(&master->lock);
>> +
>> + rc = fsi_i2c_reset(master, port->port);
>> +
>> + mutex_unlock(&master->lock);
>> + return rc;
>> +}
>> +
>> +static struct i2c_bus_recovery_info fsi_i2c_bus_recovery_info = {
>> + .recover_bus = fsi_i2c_recover_bus,
>> +};
>> +
> This all won't have any effect since you never call i2c_recover_bus
> which calls back into i2c_bus_recovery_info callbacks.
Ah, I thought there would be some use of this in the core or in client
drivers, or some ioctl interface. Would there be any outside users of
these callbacks in the future?
On 06/25/2018 09:38 PM, Wolfram Sang wrote:
> On Wed, Jun 13, 2018 at 02:36:16PM -0500, Eddie James wrote:
>> Add abort procedure for failed transfers. Add engine and bus reset
>> procedures to recover from as many faults as possible.
> I think this is a way too aggressive recovery. Your are doing the 9
> pulse toggles basically on any error while this is only when the device
> keeps SDA low and you want to recover from that. If SDA is not stuck
> low, sending a STOP should do. Or do you have a known case where this is
> not going to work?
It is aggressive, but I don't see the harm in doing this on every error.
There are some other error conditions with this hardware which may
require the clock toggling, such as "bus arbitration lost." I think this
is the safest option for this hardware, and this routine has been tested
for many years.
>
> Also, you implement the pulse toggling manually. Can't you just populate
> {get|set}_{scl|sda} and use the generic routine we have in the core?
I see that the generic implementation breaks the loop if it sees the
clock isn't high after setting it, or if SDA goes high. I think it's
safer to finish the reset for our hardware. Plus, we actually have
different registers for setting 0 or 1 to the clock/data, so we save
some cpu cycles by doing it directly instead of implementing set_scl/sda
and having to check val every time :)
If you feel very strongly that this recovery procedure needs to be
reduced, then I will work on that and have to do some extensive testing.
Thanks!
Eddie
>
On 06/25/2018 09:39 PM, Wolfram Sang wrote:
> On Wed, Jun 13, 2018 at 02:36:12PM -0500, Eddie James wrote:
>> This series adds an algorithm for an I2C master physically located on an FSI
>> slave device. The I2C master has multiple ports, each of which may be connected
>> to an I2C slave. Access to the I2C master registers is achieved over FSI bus.
>>
>> Due to the multi-port nature of the I2C master, the driver instantiates a new
>> I2C adapter for each port connected to a slave. The connected ports should be
>> defined in the device tree under the I2C master device.
> Thanks for this series and your patience.
>
> While I can see why it also helps reviewing to send it as a series of
> multiple patches, I consider applying the driver itself as just one
> hunk. I am not decided on this yet.
>
> I have a few comments, especially about recovery. I replied to the
> relevant patches with more detail.
>
> Also, are you (or someone from your company) willing to maintain the
> driver? Then, an addition to MAINTAINERS would be much appreciated.
Thanks for the review Wolfram! I addressed your comments about recovery,
please let me know what you think. I will fix the email for the dt patch
and make an addition to MAINTAINERS for the next version.
Thanks,
Eddie
>
> Thanks,
>
> Wolfram
>
Hi Eddie,
> > I think this is a way too aggressive recovery. Your are doing the 9
> > pulse toggles basically on any error while this is only when the device
> > keeps SDA low and you want to recover from that. If SDA is not stuck
> > low, sending a STOP should do. Or do you have a known case where this is
> > not going to work?
>
> It is aggressive, but I don't see the harm in doing this on every error.
Well, as it happens, I just fixed such a case. Please check these patch
series and elinux wiki pages:
===
(new fault injector)
[PATCH v2 0/2] i2c: gpio: fault-injector: add new injector
(actual recovery fix)
[PATCH 0/2] i2c: recovery: make sure pulses are not misinterpreted
===
And here is the new elinux wiki page to describe my findings:
https://elinux.org/Tests:I2C-bus-recovery-write-byte-fix
Also, the previous pages have been updated to reflect the latest status:
https://elinux.org/Tests:I2C-fault-injection
https://elinux.org/Tests:I2C-bus-recovery
To sum it up: This is a proven case where uncontrolled bus recovery can
result into a bogus write!
> There are some other error conditions with this hardware which may require
> the clock toggling, such as "bus arbitration lost." I think this is the
Why is that? In my understanding, recovery is *only* needed when SDA is
stuck low. If SDA is high, sending STOP should do. If not, it needs to
be researched why.
> safest option for this hardware, and this routine has been tested for many
> years.
I remember having a similar argument with Joakim Tjernlund a while ago.
I recently re-read our argument, yet I still keep my position: I don't
want to do $random things to recover, just a tested and well understood
procedure. And in that thread, I was never given a test case.
> >
> > Also, you implement the pulse toggling manually. Can't you just populate
> > {get|set}_{scl|sda} and use the generic routine we have in the core?
>
> I see that the generic implementation breaks the loop if it sees the clock
> isn't high after setting it, or if SDA goes high. I think it's safer to
> finish the reset for our hardware. Plus, we actually have different
Why do you think it is safer? What is the test case for that? I think
one really should do check SDA! See above, you might trigger a write
otherwise. If this breaks something for you, I am looking forward to
discuss it.
> registers for setting 0 or 1 to the clock/data, so we save some cpu cycles
> by doing it directly instead of implementing set_scl/sda and having to check
> val every time :)
Correctness comes above all here. And I am afraid your implementation is
not correct.
> If you feel very strongly that this recovery procedure needs to be reduced,
> then I will work on that and have to do some extensive testing.
I am open for discussion, yet I also feel strong about it. The reason
why the recovery procedure is moved into the core is to have one working
and understood bit-banging algorithm which all drivers can rely on. If
all drivers implement their custom version, they might miss gory details
like the above write_byte fix.
I do understand this might cause testing effort for you, I am sorry for
the delay it causes. However, my goal as a maintainer is to have a
reliable recovery mechanism, for your driver as well as for all drivers.
I hope this is understandable. BTW if you want this driver upstream
soon, then it may be an idea to resend it without any bus recovery and
then we can work on it incrementally.
Kind regards and thanks,
Wolfram
> > This all won't have any effect since you never call i2c_recover_bus
> > which calls back into i2c_bus_recovery_info callbacks.
>
> Ah, I thought there would be some use of this in the core or in client
> drivers, or some ioctl interface. Would there be any outside users of these
> callbacks in the future?
No. i2c_recover_bus() is only to recover from stalled SDA and the only
place where this can be detected is inside the I2C master driver. It is
not an event which can be controlled by a user.
> > > + if (msg->flags & I2C_M_RD)
> > > + cmd |= I2C_CMD_READ;
> > Since you support MANGLING, I'd think you can easily support
> > I2C_M_REV_DIR_ADDR here, too?
>
> Hm, I don't really understand the purpose of that flag. From the docs:
>
> This toggles the Rd/Wr flag. That is, if you want to do a write, but
> need to emit an Rd instead of a Wr, or vice versa, you set this
> flag. For example:
> S Addr Rd [A] Data [A] Data [A] ... [A] Data [A] P
>
> I don't think our hardware supports this type of operation.
I'd think something like this should do:
if (msg->flags & I2C_M_REV_DIR_ADDR)
cmd ^= I2C_CMD_READ;
On 07/02/2018 01:15 PM, Wolfram Sang wrote:
> Hi Eddie,
>
>>> I think this is a way too aggressive recovery. Your are doing the 9
>>> pulse toggles basically on any error while this is only when the device
>>> keeps SDA low and you want to recover from that. If SDA is not stuck
>>> low, sending a STOP should do. Or do you have a known case where this is
>>> not going to work?
>> It is aggressive, but I don't see the harm in doing this on every error.
> Well, as it happens, I just fixed such a case. Please check these patch
> series and elinux wiki pages:
>
> ===
>
> (new fault injector)
> [PATCH v2 0/2] i2c: gpio: fault-injector: add new injector
>
> (actual recovery fix)
> [PATCH 0/2] i2c: recovery: make sure pulses are not misinterpreted
>
> ===
>
> And here is the new elinux wiki page to describe my findings:
>
> https://elinux.org/Tests:I2C-bus-recovery-write-byte-fix
>
> Also, the previous pages have been updated to reflect the latest status:
>
> https://elinux.org/Tests:I2C-fault-injection
> https://elinux.org/Tests:I2C-bus-recovery
>
> To sum it up: This is a proven case where uncontrolled bus recovery can
> result into a bogus write!
>
>> There are some other error conditions with this hardware which may require
>> the clock toggling, such as "bus arbitration lost." I think this is the
> Why is that? In my understanding, recovery is *only* needed when SDA is
> stuck low. If SDA is high, sending STOP should do. If not, it needs to
> be researched why.
>
>> safest option for this hardware, and this routine has been tested for many
>> years.
> I remember having a similar argument with Joakim Tjernlund a while ago.
> I recently re-read our argument, yet I still keep my position: I don't
> want to do $random things to recover, just a tested and well understood
> procedure. And in that thread, I was never given a test case.
>
>>> Also, you implement the pulse toggling manually. Can't you just populate
>>> {get|set}_{scl|sda} and use the generic routine we have in the core?
>> I see that the generic implementation breaks the loop if it sees the clock
>> isn't high after setting it, or if SDA goes high. I think it's safer to
>> finish the reset for our hardware. Plus, we actually have different
> Why do you think it is safer? What is the test case for that? I think
> one really should do check SDA! See above, you might trigger a write
> otherwise. If this breaks something for you, I am looking forward to
> discuss it.
>
>> registers for setting 0 or 1 to the clock/data, so we save some cpu cycles
>> by doing it directly instead of implementing set_scl/sda and having to check
>> val every time :)
> Correctness comes above all here. And I am afraid your implementation is
> not correct.
>
>> If you feel very strongly that this recovery procedure needs to be reduced,
>> then I will work on that and have to do some extensive testing.
> I am open for discussion, yet I also feel strong about it. The reason
> why the recovery procedure is moved into the core is to have one working
> and understood bit-banging algorithm which all drivers can rely on. If
> all drivers implement their custom version, they might miss gory details
> like the above write_byte fix.
>
> I do understand this might cause testing effort for you, I am sorry for
> the delay it causes. However, my goal as a maintainer is to have a
> reliable recovery mechanism, for your driver as well as for all drivers.
>
> I hope this is understandable. BTW if you want this driver upstream
> soon, then it may be an idea to resend it without any bus recovery and
> then we can work on it incrementally.
Thanks for the details. I have sent up a new series which will only do
the bus reset if SDA is low. With our current hardware configuration,
this *should* be sufficient to recover all the possible errors. However,
there are configurations where it will not be enough, in which case
getting the data line stuck high or clock line stuck either high or low
can occur, necessitating the full reset. But since I can't demonstrate
those at the moment, I can't argue to include that now :)
Thanks again,
Eddie
>
> Kind regards and thanks,
>
> Wolfram
On 07/02/2018 01:24 PM, Wolfram Sang wrote:
>>>> + if (msg->flags & I2C_M_RD)
>>>> + cmd |= I2C_CMD_READ;
>>> Since you support MANGLING, I'd think you can easily support
>>> I2C_M_REV_DIR_ADDR here, too?
>> Hm, I don't really understand the purpose of that flag. From the docs:
>>
>> This toggles the Rd/Wr flag. That is, if you want to do a write, but
>> need to emit an Rd instead of a Wr, or vice versa, you set this
>> flag. For example:
>> S Addr Rd [A] Data [A] Data [A] ... [A] Data [A] P
>>
>> I don't think our hardware supports this type of operation.
> I'd think something like this should do:
>
> if (msg->flags & I2C_M_REV_DIR_ADDR)
> cmd ^= I2C_CMD_READ;
I meant that the hardware cannot interpret this, it would be a
meaningless command unfortunately.
>
> I meant that the hardware cannot interpret this, it would be a meaningless
> command unfortunately.
I see. Thanks for double-checking.
Eddie,
> Thanks for the details. I have sent up a new series which will only do the
> bus reset if SDA is low. With our current hardware configuration, this
Thanks.
> *should* be sufficient to recover all the possible errors. However, there
> are configurations where it will not be enough, in which case getting the
> data line stuck high or clock line stuck either high or low can occur,
> necessitating the full reset. But since I can't demonstrate those at the
> moment, I can't argue to include that now :)
For the record, I am *really* interested in these cases. Just from
reading the above I wonder how SDA can stuck high when being open drain,
and how you will create 9 SCL pulses if SCL is stuck low. But if we have
a test case, we will figure out something together.
Thanks,
Wolfram