* [PATCH v9 1/7] dt-bindings: i2c: Add FSI-attached I2C master dt binding documentation
2018-06-04 19:00 [PATCH v9 0/7] i2c: Add FSI-attached I2C master algorithm Eddie James
@ 2018-06-04 19:00 ` Eddie James
2018-06-04 19:00 ` [PATCH v9 2/7] i2c: Add FSI-attached I2C master algorithm Eddie James
` (6 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Eddie James @ 2018-06-04 19:00 UTC (permalink / raw)
To: linux-i2c
Cc: linux-kernel, devicetree, wsa, robh+dt, benh, joel, mark.rutland,
gregkh, rdunlap, andy.shevchenko, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v9 2/7] i2c: Add FSI-attached I2C master algorithm
2018-06-04 19:00 [PATCH v9 0/7] i2c: Add FSI-attached I2C master algorithm Eddie James
2018-06-04 19:00 ` [PATCH v9 1/7] dt-bindings: i2c: Add FSI-attached I2C master dt binding documentation Eddie James
@ 2018-06-04 19:00 ` Eddie James
2018-06-04 19:21 ` Andy Shevchenko
2018-06-04 19:00 ` [PATCH v9 3/7] i2c: fsi: Add port structures Eddie James
` (5 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Eddie James @ 2018-06-04 19:00 UTC (permalink / raw)
To: linux-i2c
Cc: linux-kernel, devicetree, wsa, robh+dt, benh, joel, mark.rutland,
gregkh, rdunlap, andy.shevchenko, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v9 2/7] i2c: Add FSI-attached I2C master algorithm
2018-06-04 19:00 ` [PATCH v9 2/7] i2c: Add FSI-attached I2C master algorithm Eddie James
@ 2018-06-04 19:21 ` Andy Shevchenko
2018-06-04 21:11 ` Eddie James
2018-06-04 23:38 ` Benjamin Herrenschmidt
0 siblings, 2 replies; 21+ messages in thread
From: Andy Shevchenko @ 2018-06-04 19:21 UTC (permalink / raw)
To: Eddie James
Cc: linux-i2c, Linux Kernel Mailing List, devicetree, Wolfram Sang,
Rob Herring, Benjamin Herrenschmidt, Joel Stanley, Mark Rutland,
Greg Kroah-Hartman, Randy Dunlap
On Mon, Jun 4, 2018 at 10:00 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> 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
Now it looks like a flags combinations.
For me as for reader would be better to see quickly a decoded line.
My proposal is to introduce something like following
_INT_ALL GENMASK()
_INT_ENABLE (_INT_ALL & ~(_FOO | _BAR))
_INT_ERR ... similar way as above ...
What do you think?
> +
> +/* 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
Ditto.
> +
> +/* 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;
Perhaps one property per line?
> +}
> +
> +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 }
0 is not required
> +};
> +
> +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
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 2/7] i2c: Add FSI-attached I2C master algorithm
2018-06-04 19:21 ` Andy Shevchenko
@ 2018-06-04 21:11 ` Eddie James
2018-06-04 23:38 ` Benjamin Herrenschmidt
1 sibling, 0 replies; 21+ messages in thread
From: Eddie James @ 2018-06-04 21:11 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-i2c, Linux Kernel Mailing List, devicetree, Wolfram Sang,
Rob Herring, Benjamin Herrenschmidt, Joel Stanley, Mark Rutland,
Greg Kroah-Hartman, Randy Dunlap
On 06/04/2018 02:21 PM, Andy Shevchenko wrote:
> On Mon, Jun 4, 2018 at 10:00 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>> 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
> Now it looks like a flags combinations.
> For me as for reader would be better to see quickly a decoded line.
>
> My proposal is to introduce something like following
>
> _INT_ALL GENMASK()
> _INT_ENABLE (_INT_ALL & ~(_FOO | _BAR))
> _INT_ERR ... similar way as above ...
>
> What do you think?
Yes I considered that. I do prefer the simplicity of just writing the
value directly. Doing GENMASK and then AND-ing out certain bits isn't
that much more understandable, in my opinion. To understand each bit
you'd still have to check the GENMASK against the bit fields defined
above. And just OR-ing together all the bits would be very lengthy for
such an expression. Up to you, I will follow your suggestion if you want.
>
>> +
>> +/* 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
> Ditto.
>
>> +
>> +/* 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;
> Perhaps one property per line?
At a quick glance through some other I2C bus drivers, it appears more
standard to fit as many on a line as possible. I'll leave it as is
unless anyone objects. I didn't get any checkpatch warning.
Thanks,
Eddie
>
>> +}
>> +
>> +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 }
> 0 is not required
>
>> +};
>> +
>> +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
>>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 2/7] i2c: Add FSI-attached I2C master algorithm
2018-06-04 19:21 ` Andy Shevchenko
2018-06-04 21:11 ` Eddie James
@ 2018-06-04 23:38 ` Benjamin Herrenschmidt
2018-06-05 9:17 ` Andy Shevchenko
2018-06-05 13:31 ` Eddie James
1 sibling, 2 replies; 21+ messages in thread
From: Benjamin Herrenschmidt @ 2018-06-04 23:38 UTC (permalink / raw)
To: Andy Shevchenko, Eddie James
Cc: linux-i2c, Linux Kernel Mailing List, devicetree, Wolfram Sang,
Rob Herring, Joel Stanley, Mark Rutland, Greg Kroah-Hartman,
Randy Dunlap
On Mon, 2018-06-04 at 22:21 +0300, Andy Shevchenko wrote:
> > +#define I2C_INT_ENABLE 0x0000ff80
> > +#define I2C_INT_ERR 0x0000fcc0
>
> Now it looks like a flags combinations.
> For me as for reader would be better to see quickly a decoded line.
>
> My proposal is to introduce something like following
>
> _INT_ALL GENMASK()
> _INT_ENABLE (_INT_ALL & ~(_FOO | _BAR))
> _INT_ERR ... similar way as above ...
>
> What do you think?
I don't think this absolutely needs to change but yes, open coding is
error prone. However I would think it more readable to use positive
logic and just list all the bits that are *set* even if it's a bit more
text:
#define I2C_INT_ERR (I2C_INT_INV_CMD |\
I2C_INT_PARITY |\
I2C_INT_BE_OVERRUN |\
.../...)
#define I2C_INT_ENABLE (I2C_INT_ERR |\
I2C_INT_DAT_REQ |\
I2C_INT_CMD_COMP)
Note: Eddie, I notice I2C_INT_BUSY is in "ERR" but not in "ENABLE", any
reason for that ?
Cheers,
Ben.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v9 2/7] i2c: Add FSI-attached I2C master algorithm
2018-06-04 23:38 ` Benjamin Herrenschmidt
@ 2018-06-05 9:17 ` Andy Shevchenko
2018-06-05 13:31 ` Eddie James
1 sibling, 0 replies; 21+ messages in thread
From: Andy Shevchenko @ 2018-06-05 9:17 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Eddie James, linux-i2c, Linux Kernel Mailing List, devicetree,
Wolfram Sang, Rob Herring, Joel Stanley, Mark Rutland,
Greg Kroah-Hartman, Randy Dunlap
On Tue, Jun 5, 2018 at 2:38 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Mon, 2018-06-04 at 22:21 +0300, Andy Shevchenko wrote:
>> > +#define I2C_INT_ENABLE 0x0000ff80
>> > +#define I2C_INT_ERR 0x0000fcc0
>>
>> Now it looks like a flags combinations.
>> For me as for reader would be better to see quickly a decoded line.
>>
>> My proposal is to introduce something like following
>>
>> _INT_ALL GENMASK()
>> _INT_ENABLE (_INT_ALL & ~(_FOO | _BAR))
>> _INT_ERR ... similar way as above ...
>>
>> What do you think?
>
> I don't think this absolutely needs to change but yes, open coding is
> error prone. However I would think it more readable to use positive
> logic and just list all the bits that are *set* even if it's a bit more
> text:
>
> #define I2C_INT_ERR (I2C_INT_INV_CMD |\
> I2C_INT_PARITY |\
> I2C_INT_BE_OVERRUN |\
> .../...)
>
> #define I2C_INT_ENABLE (I2C_INT_ERR |\
> I2C_INT_DAT_REQ |\
> I2C_INT_CMD_COMP)
Yep, it's fine.
I prefered though slightly different style (not putting first value on
the same line with #define), but it doesn't matter.
>
> Note: Eddie, I notice I2C_INT_BUSY is in "ERR" but not in "ENABLE", any
> reason for that ?
Exactly the reason why I payid attention on these values.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v9 2/7] i2c: Add FSI-attached I2C master algorithm
2018-06-04 23:38 ` Benjamin Herrenschmidt
2018-06-05 9:17 ` Andy Shevchenko
@ 2018-06-05 13:31 ` Eddie James
1 sibling, 0 replies; 21+ messages in thread
From: Eddie James @ 2018-06-05 13:31 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Andy Shevchenko
Cc: linux-i2c, Linux Kernel Mailing List, devicetree, Wolfram Sang,
Rob Herring, Joel Stanley, Mark Rutland, Greg Kroah-Hartman,
Randy Dunlap
On 06/04/2018 06:38 PM, Benjamin Herrenschmidt wrote:
> On Mon, 2018-06-04 at 22:21 +0300, Andy Shevchenko wrote:
>>> +#define I2C_INT_ENABLE 0x0000ff80
>>> +#define I2C_INT_ERR 0x0000fcc0
>> Now it looks like a flags combinations.
>> For me as for reader would be better to see quickly a decoded line.
>>
>> My proposal is to introduce something like following
>>
>> _INT_ALL GENMASK()
>> _INT_ENABLE (_INT_ALL & ~(_FOO | _BAR))
>> _INT_ERR ... similar way as above ...
>>
>> What do you think?
> I don't think this absolutely needs to change but yes, open coding is
> error prone. However I would think it more readable to use positive
> logic and just list all the bits that are *set* even if it's a bit more
> text:
>
> #define I2C_INT_ERR (I2C_INT_INV_CMD |\
> I2C_INT_PARITY |\
> I2C_INT_BE_OVERRUN |\
> .../...)
>
> #define I2C_INT_ENABLE (I2C_INT_ERR |\
> I2C_INT_DAT_REQ |\
> I2C_INT_CMD_COMP)
>
> Note: Eddie, I notice I2C_INT_BUSY is in "ERR" but not in "ENABLE", any
> reason for that ?
Yes, we don't want to enable an interrupt if I2C gets into the busy
state, as that happens during every transfer. However it would likely
be an error condition if we get that when the transfer is supposed to be
complete. These were from the legacy driver... I just realized that
neither are actually being used in this driver, so I will drop them.
Thanks,
Eddie
>
> Cheers,
> Ben.
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v9 3/7] i2c: fsi: Add port structures
2018-06-04 19:00 [PATCH v9 0/7] i2c: Add FSI-attached I2C master algorithm Eddie James
2018-06-04 19:00 ` [PATCH v9 1/7] dt-bindings: i2c: Add FSI-attached I2C master dt binding documentation Eddie James
2018-06-04 19:00 ` [PATCH v9 2/7] i2c: Add FSI-attached I2C master algorithm Eddie James
@ 2018-06-04 19:00 ` Eddie James
2018-06-04 19:17 ` Andy Shevchenko
2018-06-04 19:00 ` [PATCH v9 4/7] i2c: fsi: Add abort and hardware reset procedures Eddie James
` (4 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Eddie James @ 2018-06-04 19:00 UTC (permalink / raw)
To: linux-i2c
Cc: linux-kernel, devicetree, wsa, robh+dt, benh, joel, mark.rutland,
gregkh, rdunlap, andy.shevchenko, 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..12130c3 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
@@ -123,6 +126,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 +187,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 +236,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 },
{ 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v9 3/7] i2c: fsi: Add port structures
2018-06-04 19:00 ` [PATCH v9 3/7] i2c: fsi: Add port structures Eddie James
@ 2018-06-04 19:17 ` Andy Shevchenko
2018-06-04 19:40 ` Eddie James
2018-06-04 23:33 ` Benjamin Herrenschmidt
0 siblings, 2 replies; 21+ messages in thread
From: Andy Shevchenko @ 2018-06-04 19:17 UTC (permalink / raw)
To: Eddie James
Cc: linux-i2c, Linux Kernel Mailing List, devicetree, Wolfram Sang,
Rob Herring, Benjamin Herrenschmidt, Joel Stanley, Mark Rutland,
Greg Kroah-Hartman, Randy Dunlap
On Mon, Jun 4, 2018 at 10:00 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> 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..12130c3 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
>
> @@ -123,6 +126,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 +187,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);
Did you consider to split this to two lines / assignments?
> + 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 +236,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);
> + }
Just to be sure, it will be called if and only if all adapters are not
busy. Correct?
> +
> + 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
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 3/7] i2c: fsi: Add port structures
2018-06-04 19:17 ` Andy Shevchenko
@ 2018-06-04 19:40 ` Eddie James
2018-06-04 23:33 ` Benjamin Herrenschmidt
1 sibling, 0 replies; 21+ messages in thread
From: Eddie James @ 2018-06-04 19:40 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-i2c, Linux Kernel Mailing List, devicetree, Wolfram Sang,
Rob Herring, Benjamin Herrenschmidt, Joel Stanley, Mark Rutland,
Greg Kroah-Hartman, Randy Dunlap
On 06/04/2018 02:17 PM, Andy Shevchenko wrote:
> On Mon, Jun 4, 2018 at 10:00 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>> 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..12130c3 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
>>
>> @@ -123,6 +126,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 +187,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);
> Did you consider to split this to two lines / assignments?
It fit on one line (< 80 chars) so I left it as-is...
>
>> + 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 +236,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);
>> + }
> Just to be sure, it will be called if and only if all adapters are not
> busy. Correct?
If I understand the code and comments correctly, i2c_del_adapter will
block until all references to the adapter device are gone. So, should be
safe to free it now.
Thanks,
Eddie
>
>> +
>> + 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
>>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 3/7] i2c: fsi: Add port structures
2018-06-04 19:17 ` Andy Shevchenko
2018-06-04 19:40 ` Eddie James
@ 2018-06-04 23:33 ` Benjamin Herrenschmidt
1 sibling, 0 replies; 21+ messages in thread
From: Benjamin Herrenschmidt @ 2018-06-04 23:33 UTC (permalink / raw)
To: Andy Shevchenko, Eddie James
Cc: linux-i2c, Linux Kernel Mailing List, devicetree, Wolfram Sang,
Rob Herring, Joel Stanley, Mark Rutland, Greg Kroah-Hartman,
Randy Dunlap
On Mon, 2018-06-04 at 22:17 +0300, Andy Shevchenko 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);
> > + }
>
> Just to be sure, it will be called if and only if all adapters are not
> busy. Correct?
Actually i2c_del_adapter() will do the right thing. It even waits until
the embedded struct device has been fully released.
As indicated by the comment in there, it should all be turned into
something a bit better, but this is what the i2c layer gives us today.
> > +
> > + 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
> >
>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v9 4/7] i2c: fsi: Add abort and hardware reset procedures
2018-06-04 19:00 [PATCH v9 0/7] i2c: Add FSI-attached I2C master algorithm Eddie James
` (2 preceding siblings ...)
2018-06-04 19:00 ` [PATCH v9 3/7] i2c: fsi: Add port structures Eddie James
@ 2018-06-04 19:00 ` Eddie James
2018-06-04 19:00 ` [PATCH v9 5/7] i2c: fsi: Add transfer implementation Eddie James
` (3 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Eddie James @ 2018-06-04 19:00 UTC (permalink / raw)
To: linux-i2c
Cc: linux-kernel, devicetree, wsa, robh+dt, benh, joel, mark.rutland,
gregkh, rdunlap, andy.shevchenko, 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 12130c3..101b9c5 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>
@@ -123,6 +125,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;
@@ -209,6 +225,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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v9 5/7] i2c: fsi: Add transfer implementation
2018-06-04 19:00 [PATCH v9 0/7] i2c: Add FSI-attached I2C master algorithm Eddie James
` (3 preceding siblings ...)
2018-06-04 19:00 ` [PATCH v9 4/7] i2c: fsi: Add abort and hardware reset procedures Eddie James
@ 2018-06-04 19:00 ` Eddie James
2018-06-04 19:14 ` Andy Shevchenko
2018-06-04 19:45 ` Peter Rosin
2018-06-04 19:00 ` [PATCH v9 6/7] i2c: fsi: Add I2C master locking Eddie James
` (2 subsequent siblings)
7 siblings, 2 replies; 21+ messages in thread
From: Eddie James @ 2018-06-04 19:00 UTC (permalink / raw)
To: linux-i2c
Cc: linux-kernel, devicetree, wsa, robh+dt, benh, joel, mark.rutland,
gregkh, rdunlap, andy.shevchenko, 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 | 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 101b9c5..4e5964e 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -150,6 +150,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,
@@ -225,6 +226,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;
@@ -388,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);
+
+ 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 0;
}
static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v9 5/7] i2c: fsi: Add transfer implementation
2018-06-04 19:00 ` [PATCH v9 5/7] i2c: fsi: Add transfer implementation Eddie James
@ 2018-06-04 19:14 ` Andy Shevchenko
2018-06-04 19:37 ` Eddie James
2018-06-04 19:45 ` Peter Rosin
1 sibling, 1 reply; 21+ messages in thread
From: Andy Shevchenko @ 2018-06-04 19:14 UTC (permalink / raw)
To: Eddie James
Cc: linux-i2c, Linux Kernel Mailing List, devicetree, Wolfram Sang,
Rob Herring, Benjamin Herrenschmidt, Joel Stanley, Mark Rutland,
Greg Kroah-Hartman, Randy Dunlap
On Mon, Jun 4, 2018 at 10:00 PM, 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.
>
> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
> ---
> 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 101b9c5..4e5964e 100644
> --- a/drivers/i2c/busses/i2c-fsi.c
> +++ b/drivers/i2c/busses/i2c-fsi.c
> @@ -150,6 +150,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,
> @@ -225,6 +226,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);
Did you consider to unify READ + ADDR as ADDR_8BIT?
> + 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)
Since you dropped one of 'else', another also is not needed.
> + 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;
> + }
Just to be sure, if by some reason write == 0 in the loop for a while,
would it become infinite loop?
> +
> + 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;
> + }
Ditto for read == 0.
> +
> + return 0;
> +}
> +
> static int fsi_i2c_reset_bus(struct fsi_i2c_master *i2c)
> {
> int i, rc;
> @@ -388,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);
> +
> + 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 0;
> }
>
> static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
> --
> 1.8.3.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 5/7] i2c: fsi: Add transfer implementation
2018-06-04 19:14 ` Andy Shevchenko
@ 2018-06-04 19:37 ` Eddie James
0 siblings, 0 replies; 21+ messages in thread
From: Eddie James @ 2018-06-04 19:37 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-i2c, Linux Kernel Mailing List, devicetree, Wolfram Sang,
Rob Herring, Benjamin Herrenschmidt, Joel Stanley, Mark Rutland,
Greg Kroah-Hartman, Randy Dunlap
On 06/04/2018 02:14 PM, Andy Shevchenko wrote:
> On Mon, Jun 4, 2018 at 10:00 PM, 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.
>>
>> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
>> ---
>> 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 101b9c5..4e5964e 100644
>> --- a/drivers/i2c/busses/i2c-fsi.c
>> +++ b/drivers/i2c/busses/i2c-fsi.c
>> @@ -150,6 +150,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,
>> @@ -225,6 +226,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);
> Did you consider to unify READ + ADDR as ADDR_8BIT?
Yes, however the result from the i2c_8bit_addr_from_msg function is not
equivalent. This hardware expects the 8-bit address to be concatenated
with the read/not write bit. Also the function returns a u8, which won't
work for addresses with bit 7 set, since it left-shifts by one.
>
>> + 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)
> Since you dropped one of 'else', another also is not needed.
True.
>
>> + 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;
>> + }
> Just to be sure, if by some reason write == 0 in the loop for a while,
> would it become infinite loop?
It's not possible that write == 0. Same for read below.
Thanks,
Eddie
>
>> +
>> + 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;
>> + }
> Ditto for read == 0.
>
>> +
>> + return 0;
>> +}
>> +
>> static int fsi_i2c_reset_bus(struct fsi_i2c_master *i2c)
>> {
>> int i, rc;
>> @@ -388,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);
>> +
>> + 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 0;
>> }
>>
>> static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
>> --
>> 1.8.3.1
>>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v9 5/7] i2c: fsi: Add transfer implementation
2018-06-04 19:00 ` [PATCH v9 5/7] i2c: fsi: Add transfer implementation Eddie James
2018-06-04 19:14 ` Andy Shevchenko
@ 2018-06-04 19:45 ` Peter Rosin
2018-06-04 21:12 ` Eddie James
1 sibling, 1 reply; 21+ messages in thread
From: Peter Rosin @ 2018-06-04 19:45 UTC (permalink / raw)
To: Eddie James, linux-i2c
Cc: linux-kernel, devicetree, wsa, robh+dt, benh, joel, mark.rutland,
gregkh, rdunlap, andy.shevchenko
On 2018-06-04 21:00, 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 <eajames@linux.vnet.ibm.com>
> ---
> 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 101b9c5..4e5964e 100644
> --- a/drivers/i2c/busses/i2c-fsi.c
> +++ b/drivers/i2c/busses/i2c-fsi.c
> @@ -150,6 +150,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,
> @@ -225,6 +226,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;
> @@ -388,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);
> +
> + 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 0;
Zero is not the return value of success for .master_xfer
Hint, "num" is your friend...
Cheers,
Peter
> }
>
> static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 5/7] i2c: fsi: Add transfer implementation
2018-06-04 19:45 ` Peter Rosin
@ 2018-06-04 21:12 ` Eddie James
0 siblings, 0 replies; 21+ messages in thread
From: Eddie James @ 2018-06-04 21:12 UTC (permalink / raw)
To: Peter Rosin, linux-i2c
Cc: linux-kernel, devicetree, wsa, robh+dt, benh, joel, mark.rutland,
gregkh, rdunlap, andy.shevchenko
On 06/04/2018 02:45 PM, Peter Rosin wrote:
> On 2018-06-04 21:00, 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 <eajames@linux.vnet.ibm.com>
>> ---
>> 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 101b9c5..4e5964e 100644
>> --- a/drivers/i2c/busses/i2c-fsi.c
>> +++ b/drivers/i2c/busses/i2c-fsi.c
>> @@ -150,6 +150,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,
>> @@ -225,6 +226,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;
>> @@ -388,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);
>> +
>> + 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 0;
> Zero is not the return value of success for .master_xfer
>
> Hint, "num" is your friend...
>
> Cheers,
> Peter
Good catch, I'll fix that.
Thanks,
Eddie
>
>> }
>>
>> static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
>>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v9 6/7] i2c: fsi: Add I2C master locking
2018-06-04 19:00 [PATCH v9 0/7] i2c: Add FSI-attached I2C master algorithm Eddie James
` (4 preceding siblings ...)
2018-06-04 19:00 ` [PATCH v9 5/7] i2c: fsi: Add transfer implementation Eddie James
@ 2018-06-04 19:00 ` Eddie James
2018-06-04 19:00 ` [PATCH v9 7/7] i2c: fsi: Add bus recovery Eddie James
2018-06-04 19:22 ` [PATCH v9 0/7] i2c: Add FSI-attached I2C master algorithm Andy Shevchenko
7 siblings, 0 replies; 21+ messages in thread
From: Eddie James @ 2018-06-04 19:00 UTC (permalink / raw)
To: linux-i2c
Cc: linux-kernel, devicetree, wsa, robh+dt, benh, joel, mark.rutland,
gregkh, rdunlap, andy.shevchenko, 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 4e5964e..6e6e2b1 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>
@@ -143,6 +144,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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v9 7/7] i2c: fsi: Add bus recovery
2018-06-04 19:00 [PATCH v9 0/7] i2c: Add FSI-attached I2C master algorithm Eddie James
` (5 preceding siblings ...)
2018-06-04 19:00 ` [PATCH v9 6/7] i2c: fsi: Add I2C master locking Eddie James
@ 2018-06-04 19:00 ` Eddie James
2018-06-04 19:22 ` [PATCH v9 0/7] i2c: Add FSI-attached I2C master algorithm Andy Shevchenko
7 siblings, 0 replies; 21+ messages in thread
From: Eddie James @ 2018-06-04 19:00 UTC (permalink / raw)
To: linux-i2c
Cc: linux-kernel, devicetree, wsa, robh+dt, benh, joel, mark.rutland,
gregkh, rdunlap, andy.shevchenko, 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 6e6e2b1..21aa50d 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v9 0/7] i2c: Add FSI-attached I2C master algorithm
2018-06-04 19:00 [PATCH v9 0/7] i2c: Add FSI-attached I2C master algorithm Eddie James
` (6 preceding siblings ...)
2018-06-04 19:00 ` [PATCH v9 7/7] i2c: fsi: Add bus recovery Eddie James
@ 2018-06-04 19:22 ` Andy Shevchenko
7 siblings, 0 replies; 21+ messages in thread
From: Andy Shevchenko @ 2018-06-04 19:22 UTC (permalink / raw)
To: Eddie James
Cc: linux-i2c, Linux Kernel Mailing List, devicetree, Wolfram Sang,
Rob Herring, Benjamin Herrenschmidt, Joel Stanley, Mark Rutland,
Greg Kroah-Hartman, Randy Dunlap
On Mon, Jun 4, 2018 at 10:00 PM, 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.
>
Thanks for an update.
With some minor comments this looks indeed excellent work!
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 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 | 721 ++++++++++++++++++++++
> 4 files changed, 773 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
^ permalink raw reply [flat|nested] 21+ messages in thread