[PATCH v8 0/7] i2c: Add FSI-attached I2C master algorithm

[PATCH v8 0/7] i2c: Add FSI-attached I2C master algorithm

[Eddie James]

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 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                      | 722 ++++++++++++++++++++++
 4 files changed, 774 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-fsi.txt
 create mode 100644 drivers/i2c/busses/i2c-fsi.c

-- 
1.8.3.1

[PATCH v8 1/7] dt-bindings: i2c: Add FSI-attached I2C master dt binding documentation

[Eddie James]

Document the bindings.

Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>>
Acked-by: Rob Herring <robh@kernel.org>
---
 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

[PATCH v8 2/7] i2c: Add FSI-attached I2C master algorithm

[Eddie James]

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 <eajames@linux.vnet.ibm.com>
---
 drivers/i2c/busses/Kconfig   |  11 ++
 drivers/i2c/busses/Makefile  |   1 +
 drivers/i2c/busses/i2c-fsi.c | 234 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 246 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..e1b183c
--- /dev/null
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -0,0 +1,234 @@
+// 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)
+
+#define I2C_INT_ENABLE		0x0000ff80
+#define I2C_INT_ERR		0x0000fcc0
+
+/* 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		0xfc800000
+#define I2C_STAT_ANY_RESP	0xff800000
+
+/* 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 },
+	{ 0 }
+};
+
+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 <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("FSI attached I2C master");
+MODULE_LICENSE("GPL");
-- 
1.8.3.1

[PATCH v8 3/7] i2c: fsi: Add port structures

[Eddie James]

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 <eajames@linux.vnet.ibm.com>
---
 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 e1b183c..be8e15c 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -17,7 +17,9 @@
 #include <linux/fsi.h>
 #include <linux/i2c.h>
 #include <linux/kernel.h>
+#include <linux/list.h>
 #include <linux/module.h>
+#include <linux/of.h>
 
 #define FSI_ENGID_I2C		0x7
 
@@ -123,6 +125,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,
@@ -176,9 +186,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;
 }
 
@@ -196,23 +235,73 @@ 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 = devm_kzalloc(dev, 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);
+			devm_kfree(dev, 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;
+
+	if (!list_empty(&i2c->ports)) {
+		list_for_each_entry(port, &i2c->ports, list) {
+			i2c_del_adapter(&port->adapter);
+		}
+	}
+
+	return 0;
+}
+
 static const struct fsi_device_id fsi_i2c_ids[] = {
 	{ FSI_ENGID_I2C, FSI_VERSION_ANY },
 	{ 0 }
@@ -224,6 +313,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

[PATCH v8 4/7] i2c: fsi: Add abort and hardware reset procedures

[Eddie James]

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 <eajames@linux.vnet.ibm.com>
---
 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 be8e15c..e5dd0f7 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>
@@ -122,6 +124,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;
@@ -208,6 +224,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 = (mode & ~I2C_MODE_PORT) | 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

[PATCH v8 5/7] i2c: fsi: Add transfer implementation

[Eddie James]

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 <eajames@linux.vnet.ibm.com>
---
 drivers/i2c/busses/i2c-fsi.c | 196 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 194 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index e5dd0f7..f24e4b9 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -149,6 +149,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,
@@ -224,6 +225,100 @@ 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;
+	else
+		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;
@@ -387,17 +482,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);
+		else
+			return fsi_i2c_write_fifo(port, msg, fifo_count);
+	}
+
+	if (status & I2C_STAT_CMD_COMP) {
+		if (port->xfrd < msg->len)
+			return -ENODATA;
+		else
+			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 0;
 }
 
 static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
-- 
1.8.3.1

[PATCH v8 6/7] i2c: fsi: Add I2C master locking

[Eddie James]

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 <eajames@linux.vnet.ibm.com>
---
 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 f24e4b9..5dad16a 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>
 
 #define FSI_ENGID_I2C		0x7
@@ -142,6 +143,7 @@ struct fsi_i2c_master {
 	struct fsi_device	*fsi;
 	u8			fifo_size;
 	struct list_head	ports;
+	struct mutex		lock;
 };
 
 struct fsi_i2c_port {
@@ -569,11 +571,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;
@@ -581,15 +586,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 0;
+unlock:
+	mutex_unlock(&master->lock);
+	return rc;
 }
 
 static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
@@ -615,6 +622,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

[PATCH v8 7/7] i2c: fsi: Add bus recovery

[Eddie James]

Bus recovery should reset the engine and force clock the bus 9 times
to recover most situations.

Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
 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 5dad16a..a8b7d89 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -605,6 +605,24 @@ static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
 		| I2C_FUNC_SMBUS_EMUL | 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,
@@ -647,6 +665,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

Re: [PATCH v8 0/7] i2c: Add FSI-attached I2C master algorithm

[Andy Shevchenko]

On Thu, May 31, 2018 at 12:07 AM, Eddie James
<eajames@linux.vnet.ibm.com> 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.

I'll comment the series later, though you have to address previous
comments first:
- understand devm_ purpose and how it works
- understand list_for_each*() macros
- discuss with maintainer a design of enumerating ports
- ... (whatever I forgot and others added) ...

> 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                      | 722 ++++++++++++++++++++++
>  4 files changed, 774 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

Re: [PATCH v8 4/7] i2c: fsi: Add abort and hardware reset procedures

[Andy Shevchenko]

On Thu, May 31, 2018 at 12:07 AM, Eddie James
<eajames@linux.vnet.ibm.com> wrote:
> Add abort procedure for failed transfers. Add engine and bus reset
> procedures to recover from as many faults as possible.
>

Few small comments, after addressing which

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>


> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
> ---
>  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 be8e15c..e5dd0f7 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>
> @@ -122,6 +124,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;
> @@ -208,6 +224,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) {

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 = (mode & ~I2C_MODE_PORT) | FIELD_PREP(I2C_MODE_PORT,
> +                                                           port);

Hmm... This would look better as two expressions

mode &= ~..._PORT;
mode |= FIELD_PREP(...);

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



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 0/7] i2c: Add FSI-attached I2C master algorithm

[Benjamin Herrenschmidt]

On Thu, 2018-05-31 at 00:31 +0300, Andy Shevchenko wrote:
> On Thu, May 31, 2018 at 12:07 AM, Eddie James
> <eajames@linux.vnet.ibm.com> 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.
> 
> I'll comment the series later, though you have to address previous
> comments first:
> - understand devm_ purpose and how it works

I think it is perfectly understood and I don't see what your problem
here is. So please be a proper civil human being an express your
concern precisely rather than with aggressive comments.

Now to clarify that specific point, devm purpose is to automatically
clean up the resources used by the device when it is torn down.

However, in this specific case, it makes sense to dispose of the port
structure explicitly because this is a failure in registering an
individual port which doesn't lead to a failure of the entire driver.

Thus not freeing it means the structure would remain allocated
uselessly until the whole driver is torn down.

> - understand list_for_each*() macros

This is a genuine misunderstanding of Eddies, I'm sure he will address
it, your tone however isn't appropriate.

> - discuss with maintainer a design of enumerating ports

I've been at that game for at least a good 2 decades. Maintainers
generally do *not* discuss design until a patch is proposed. I even
still try every now and then, maintainers are like lawyers, they don't
want to tell you what to do in case they still want to reject it after
seeing it later :-) I know I've been one of them for long enough.

If you have specific issues with how this is done, please express them
clearly. It's quite possible that there's some better way to do what
Eddie is doing here, but without *construtive* feedback this is
pointless. 

> - ... (whatever I forgot and others added) ...

I believe he addressed most of your comments. There is one mistake left
(the list_for_each) which will be easily fixed. As for the rest, it's
just ramblings unless you or Wolfram can propose a better alternative
in which case I'm sure Eddie will be happy to implement it.

I'm disappointed here because we have an example of somebody rather new
producing what is overall pretty damn good code, despite a few corner
issues, and being (again) treated like crap.

This isn't the right way to operate, and I believe this has been made
clear many times before.

Cheers,
Ben.

> > 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                      | 722 ++++++++++++++++++++++
> >  4 files changed, 774 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-fsi.txt
> >  create mode 100644 drivers/i2c/busses/i2c-fsi.c
> > 
> > --
> > 1.8.3.1
> > 
> 
> 
> 

Re: [PATCH v8 0/7] i2c: Add FSI-attached I2C master algorithm

[Andy Shevchenko]

On Thu, May 31, 2018 at 1:42 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Thu, 2018-05-31 at 00:31 +0300, Andy Shevchenko wrote:
>> On Thu, May 31, 2018 at 12:07 AM, Eddie James
>> <eajames@linux.vnet.ibm.com> wrote:

>> I'll comment the series later, though you have to address previous
>> comments first:
>> - understand devm_ purpose and how it works
>
> I think it is perfectly understood and I don't see what your problem
> here is. So please be a proper civil human being an express your
> concern precisely rather than with aggressive comments.

I apologize for this kind of tone, let's assume it was a bad day.

> Now to clarify that specific point, devm purpose is to automatically
> clean up the resources used by the device when it is torn down.
>
> However, in this specific case, it makes sense to dispose of the port
> structure explicitly because this is a failure in registering an
> individual port which doesn't lead to a failure of the entire driver.
>
> Thus not freeing it means the structure would remain allocated
> uselessly until the whole driver is torn down.

Yep, so, why do we care? If it holds few hundreds of bytes, can't we
live with it?
If no, the devm_k*alloc() is a wrong choice in the first place.

>> - discuss with maintainer a design of enumerating ports
>
> I've been at that game for at least a good 2 decades. Maintainers
> generally do *not* discuss design until a patch is proposed. I even
> still try every now and then, maintainers are like lawyers, they don't
> want to tell you what to do in case they still want to reject it after
> seeing it later :-) I know I've been one of them for long enough.
>
> If you have specific issues with how this is done, please express them
> clearly. It's quite possible that there's some better way to do what
> Eddie is doing here, but without *construtive* feedback this is
> pointless.

It feels like you duplicate approach which is done in OF generic case.
That is my concern. Though, if Wolfram is telling that is OK, I have
no objections.

> I'm disappointed here because we have an example of somebody rather new
> producing what is overall pretty damn good code,

That is true. His code much better than many I have seen before.

> despite a few corner
> issues, and being (again) treated like crap.

Sorry for that, life is harsh.

> This isn't the right way to operate, and I believe this has been made
> clear many times before.

Yes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 5/7] i2c: fsi: Add transfer implementation

[Andy Shevchenko]

On Thu, May 31, 2018 at 12:07 AM, Eddie James
<eajames@linux.vnet.ibm.com> wrote:
> Execute I2C transfers from the FSI-attached I2C master. Use polling
> instead of interrupts as we have no hardware IRQ over FSI.
>

Few nitpicks, after addressing them (field definition is up to you though)

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>


> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
> ---
>  drivers/i2c/busses/i2c-fsi.c | 196 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 194 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
> index e5dd0f7..f24e4b9 100644
> --- a/drivers/i2c/busses/i2c-fsi.c
> +++ b/drivers/i2c/busses/i2c-fsi.c
> @@ -149,6 +149,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,
> @@ -224,6 +225,100 @@ 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;

(1)

> +
> +       if (stop || msg->flags & I2C_M_STOP)
> +               cmd |= I2C_CMD_WITH_STOP;
> +

> +       cmd |= FIELD_PREP(I2C_CMD_ADDR, msg->addr >> 1);

(1) + this looks like:

 // #define I2C_CMD_ADDR           GENMASK(23, 17)
 // #define I2C_CMD_READ           BIT(16)

#define I2C_CMD_8BIT_ADDR           GENMASK(23, 16)
// or even drop I2C_CMD_READ completely

   cmd |= FIELD_PREP(I2C_CMD_8BIT_ADDR, i2c_8bit_addr_from_msg(msg));

> +       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;
> +       else
> +               return op_bytes;

If you have pattern like

if {
 ...
 return;
} else {
 ...
}

'else' is redundant.

> +}

Overall, I think rounddown_pow_of_two() makes sense here.
Though it's your choice.

> +
> +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;
> @@ -387,17 +482,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);

> +               else
> +                       return fsi_i2c_write_fifo(port, msg, fifo_count);

Redundant 'else'

> +       }
> +
> +       if (status & I2C_STAT_CMD_COMP) {
> +               if (port->xfrd < msg->len)
> +                       return -ENODATA;

> +               else
> +                       return msg->len;

Ditto.

> +       }
> +
> +       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) {

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 0;
>  }
>
>  static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
> --
> 1.8.3.1
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 0/7] i2c: Add FSI-attached I2C master algorithm

[Benjamin Herrenschmidt]

On Thu, 2018-05-31 at 09:29 +0300, Andy Shevchenko wrote:
> > If you have specific issues with how this is done, please express them
> > clearly. It's quite possible that there's some better way to do what
> > Eddie is doing here, but without *construtive* feedback this is
> > pointless.
> 
> It feels like you duplicate approach which is done in OF generic case.
> That is my concern. Though, if Wolfram is telling that is OK, I have
> no objections.

THe OF generic case is about discovering slaves underneath a port, not
ports inside of a mulit-port master.

I am not aware of a generic mechanism for the latter. We *could* make
the ports sub-devices but it gets messy then to arbitrate the
communication and deal with the common part. I've seen (and written)
multi-port masters in the past that use a similar approach to what
Eddie's doing and it works fine.

> > I'm disappointed here because we have an example of somebody rather new
> > producing what is overall pretty damn good code,
> 
> That is true. His code much better than many I have seen before

Thanks. Also thanks for taking the time to review.

> > despite a few corner
> > issues, and being (again) treated like crap.
> 
> Sorry for that, life is harsh.
> 
> > This isn't the right way to operate, and I believe this has been made
> > clear many times before.
> 
> Yes.


Cheers,
Ben.