[PATCH v8 0/5] i2c: aspeed: added driver for Aspeed I2C

[PATCH v8 0/5] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

Addressed comments from:
  - Ben in: http://www.spinics.net/lists/devicetree/msg174576.html
    and: https://www.spinics.net/lists/kernel/msg2496151.html
  - Wolfram in: https://www.spinics.net/lists/kernel/msg2504011.html
  - Cedric in: https://www.spinics.net/lists/kernel/msg2506531.html

Changes since previous update:
  - ACK IRQ status bits before doing anything else
  - Added multi-master device tree property
  - Do not send STOP commands after interrupt errors
  - Fix SMBUS_QUICK emulation handling
  - Removed highspeed clock code (I will do it in a later patch set)
  - Use the platform_device name for the adapter name
  - Reset for all failed recoveries
  - Removed the "__" prefix from all of the non-thread safe functions

As before, tested on Aspeed 2500 evaluation board and a real platform with an
Aspeed 2520.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v8 1/5] irqchip/aspeed-i2c-ic: binding docs for Aspeed I2C Interrupt Controller

[Brendan Higgins]

Added device tree binding documentation for Aspeed I2C Interrupt
Controller.

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
Added in v6:
  - Pulled "aspeed_i2c_controller" out into a interrupt controller since that is
    what it actually does.
Changes for v7:
  - None
Changes for v8:
  - None
---
 .../interrupt-controller/aspeed,ast2400-i2c-ic.txt | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-i2c-ic.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-i2c-ic.txt b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-i2c-ic.txt
new file mode 100644
index 000000000000..033cc82e5684
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-i2c-ic.txt
@@ -0,0 +1,25 @@
+Device tree configuration for the I2C Interrupt Controller on the AST24XX and
+AST25XX SoCs.
+
+Required Properties:
+- #address-cells	: should be 1
+- #size-cells 		: should be 1
+- #interrupt-cells 	: should be 1
+- compatible 		: should be "aspeed,ast2400-i2c-ic"
+			  or "aspeed,ast2500-i2c-ic"
+- reg			: address start and range of controller
+- interrupts		: interrupt number
+- interrupt-controller	: denotes that the controller receives and fires
+			  new interrupts for child busses
+
+Example:
+
+i2c_ic: interrupt-controller@0 {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	#interrupt-cells = <1>;
+	compatible = "aspeed,ast2400-i2c-ic";
+	reg = <0x0 0x40>;
+	interrupts = <12>;
+	interrupt-controller;
+};
-- 
2.13.0.219.gdb65acc882-goog

Re: [PATCH v8 1/5] irqchip/aspeed-i2c-ic: binding docs for Aspeed I2C Interrupt Controller

[Rob Herring]

On Wed, May 24, 2017 at 11:49:21PM -0700, Brendan Higgins wrote:
> Added device tree binding documentation for Aspeed I2C Interrupt
> Controller.
> 
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
> Added in v6:
>   - Pulled "aspeed_i2c_controller" out into a interrupt controller since that is
>     what it actually does.
> Changes for v7:
>   - None
> Changes for v8:
>   - None

I already acked this. Please add acks when posting new versions.

Rob

Re: [PATCH v8 1/5] irqchip/aspeed-i2c-ic: binding docs for Aspeed I2C Interrupt Controller

[Brendan Higgins]

Sorry, I wasn't sure. Will do in the future.

On Tue, May 30, 2017 at 2:34 PM, Rob Herring <robh@kernel.org> wrote:
> On Wed, May 24, 2017 at 11:49:21PM -0700, Brendan Higgins wrote:
>> Added device tree binding documentation for Aspeed I2C Interrupt
>> Controller.
>>
>> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
>> ---
>> Added in v6:
>>   - Pulled "aspeed_i2c_controller" out into a interrupt controller since that is
>>     what it actually does.
>> Changes for v7:
>>   - None
>> Changes for v8:
>>   - None
>
> I already acked this. Please add acks when posting new versions.
>
> Rob

[PATCH v8 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed

[Brendan Higgins]

The Aspeed 24XX/25XX chips share a single hardware interrupt across 14
separate I2C busses. This adds a dummy irqchip which maps the single
hardware interrupt to software interrupts for each of the busses.

Signed-off-by: Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
Added in v6:
  - Pulled "aspeed_i2c_controller" out into a interrupt controller since that is
    what it actually does.
Changes for v7:
  - Renamed irq domain for consistency
Changes for v8:
  - None
---
 drivers/irqchip/Makefile            |   2 +-
 drivers/irqchip/irq-aspeed-i2c-ic.c | 102 ++++++++++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+), 1 deletion(-)
 create mode 100644 drivers/irqchip/irq-aspeed-i2c-ic.c

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b64c59b838a0..e067f9839b33 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -73,6 +73,6 @@ obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
 obj-$(CONFIG_MVEBU_PIC)			+= irq-mvebu-pic.o
 obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
 obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
-obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
+obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
diff --git a/drivers/irqchip/irq-aspeed-i2c-ic.c b/drivers/irqchip/irq-aspeed-i2c-ic.c
new file mode 100644
index 000000000000..a36fb09c10c2
--- /dev/null
+++ b/drivers/irqchip/irq-aspeed-i2c-ic.c
@@ -0,0 +1,102 @@
+/*
+ *  Aspeed 24XX/25XX I2C Interrupt Controller.
+ *
+ *  Copyright (C) 2012-2017 ASPEED Technology Inc.
+ *  Copyright 2017 IBM Corporation
+ *  Copyright 2017 Google, Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ */
+
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+
+
+#define ASPEED_I2C_IC_NUM_BUS 14
+
+struct aspeed_i2c_ic {
+	void __iomem		*base;
+	int			parent_irq;
+	struct irq_domain	*irq_domain;
+};
+
+/*
+ * The aspeed chip provides a single hardware interrupt for all of the I2C
+ * busses, so we use a dummy interrupt chip to translate this single interrupt
+ * into multiple interrupts, each associated with a single I2C bus.
+ */
+static void aspeed_i2c_ic_irq_handler(struct irq_desc *desc)
+{
+	struct aspeed_i2c_ic *i2c_ic = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned long bit, status;
+	unsigned int bus_irq;
+
+	chained_irq_enter(chip, desc);
+	status = readl(i2c_ic->base);
+	for_each_set_bit(bit, &status, ASPEED_I2C_IC_NUM_BUS) {
+		bus_irq = irq_find_mapping(i2c_ic->irq_domain, bit);
+		generic_handle_irq(bus_irq);
+	}
+	chained_irq_exit(chip, desc);
+}
+
+/*
+ * Set simple handler and mark IRQ as valid. Nothing interesting to do here
+ * since we are using a dummy interrupt chip.
+ */
+static int aspeed_i2c_ic_map_irq_domain(struct irq_domain *domain,
+					unsigned int irq, irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops aspeed_i2c_ic_irq_domain_ops = {
+	.map = aspeed_i2c_ic_map_irq_domain,
+};
+
+static int __init aspeed_i2c_ic_of_init(struct device_node *node,
+					struct device_node *parent)
+{
+	struct aspeed_i2c_ic *i2c_ic;
+
+	i2c_ic = kzalloc(sizeof(*i2c_ic), GFP_KERNEL);
+	if (!i2c_ic)
+		return -ENOMEM;
+
+	i2c_ic->base = of_iomap(node, 0);
+	if (IS_ERR(i2c_ic->base))
+		return PTR_ERR(i2c_ic->base);
+
+	i2c_ic->parent_irq = irq_of_parse_and_map(node, 0);
+	if (i2c_ic->parent_irq < 0)
+		return i2c_ic->parent_irq;
+
+	i2c_ic->irq_domain = irq_domain_add_linear(
+			node, ASPEED_I2C_IC_NUM_BUS,
+			&aspeed_i2c_ic_irq_domain_ops, NULL);
+	if (!i2c_ic->irq_domain)
+		return -ENOMEM;
+
+	i2c_ic->irq_domain->name = "aspeed-i2c-domain";
+
+	irq_set_chained_handler_and_data(i2c_ic->parent_irq,
+					 aspeed_i2c_ic_irq_handler, i2c_ic);
+
+	pr_info("i2c controller registered, irq %d\n", i2c_ic->parent_irq);
+
+	return 0;
+}
+
+IRQCHIP_DECLARE(ast2400_i2c_ic, "aspeed,ast2400-i2c-ic", aspeed_i2c_ic_of_init);
+IRQCHIP_DECLARE(ast2500_i2c_ic, "aspeed,ast2500-i2c-ic", aspeed_i2c_ic_of_init);
-- 
2.13.0.219.gdb65acc882-goog

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

[PATCH v8 5/5] i2c: aspeed: added slave support for Aspeed I2C driver

[Brendan Higgins]

Added slave support for Aspeed I2C controller. Supports fourteen busses
present in AST24XX and AST25XX BMC SoCs by Aspeed.

Signed-off-by: Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
Added in v6:
  - Pulled slave support out of initial driver commit into its own commit.
  - No longer arbitrarily restrict bus to be slave xor master.
Changes for v7:
  - Added hardware reset function
  - Marked functions that need to be called with the lock held as "unlocked"
  - Did some cleanup
Changes for v8:
  - None
---
 drivers/i2c/busses/i2c-aspeed.c | 202 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 202 insertions(+)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 3eb1252442d7..0ab29f5029fb 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -49,6 +49,7 @@
 #define ASPEED_I2CD_SDA_DRIVE_1T_EN			BIT(8)
 #define ASPEED_I2CD_M_SDA_DRIVE_1T_EN			BIT(7)
 #define ASPEED_I2CD_M_HIGH_SPEED_EN			BIT(6)
+#define ASPEED_I2CD_SLAVE_EN				BIT(1)
 #define ASPEED_I2CD_MASTER_EN				BIT(0)
 
 /* 0x04 : I2CD Clock and AC Timing Control Register #1 */
@@ -69,6 +70,7 @@
  */
 #define ASPEED_I2CD_INTR_SDA_DL_TIMEOUT			BIT(14)
 #define ASPEED_I2CD_INTR_BUS_RECOVER_DONE		BIT(13)
+#define ASPEED_I2CD_INTR_SLAVE_MATCH			BIT(7)
 #define ASPEED_I2CD_INTR_SCL_TIMEOUT			BIT(6)
 #define ASPEED_I2CD_INTR_ABNORMAL			BIT(5)
 #define ASPEED_I2CD_INTR_NORMAL_STOP			BIT(4)
@@ -106,6 +108,9 @@
 #define ASPEED_I2CD_M_TX_CMD				BIT(1)
 #define ASPEED_I2CD_M_START_CMD				BIT(0)
 
+/* 0x18 : I2CD Slave Device Address Register   */
+#define ASPEED_I2CD_DEV_ADDR_MASK			GENMASK(6, 0)
+
 enum aspeed_i2c_master_state {
 	ASPEED_I2C_MASTER_START,
 	ASPEED_I2C_MASTER_TX_FIRST,
@@ -116,6 +121,15 @@ enum aspeed_i2c_master_state {
 	ASPEED_I2C_MASTER_INACTIVE,
 };
 
+enum aspeed_i2c_slave_state {
+	ASPEED_I2C_SLAVE_START,
+	ASPEED_I2C_SLAVE_READ_REQUESTED,
+	ASPEED_I2C_SLAVE_READ_PROCESSED,
+	ASPEED_I2C_SLAVE_WRITE_REQUESTED,
+	ASPEED_I2C_SLAVE_WRITE_RECEIVED,
+	ASPEED_I2C_SLAVE_STOP,
+};
+
 struct aspeed_i2c_bus {
 	struct i2c_adapter		adap;
 	struct device			*dev;
@@ -132,6 +146,10 @@ struct aspeed_i2c_bus {
 	size_t				msgs_count;
 	bool				send_stop;
 	int				cmd_err;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	struct i2c_client		*slave;
+	enum aspeed_i2c_slave_state	slave_state;
+#endif /* CONFIG_I2C_SLAVE */
 };
 
 static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus);
@@ -203,6 +221,110 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
 	return aspeed_i2c_reset(bus);
 }
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
+{
+	u32 command, irq_status, status_ack = 0;
+	struct i2c_client *slave = bus->slave;
+	bool irq_handled = true;
+	u8 value;
+
+	spin_lock(&bus->lock);
+	if (!slave) {
+		irq_handled = false;
+		goto out;
+	}
+
+	command = readl(bus->base + ASPEED_I2C_CMD_REG);
+	irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+
+	/* Slave was requested, restart state machine. */
+	if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
+		status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
+		bus->slave_state = ASPEED_I2C_SLAVE_START;
+	}
+
+	/* Slave is not currently active, irq was for someone else. */
+	if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
+		irq_handled = false;
+		goto out;
+	}
+
+	dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
+		irq_status, command);
+
+	/* Slave was sent something. */
+	if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
+		value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
+		/* Handle address frame. */
+		if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
+			if (value & 0x1)
+				bus->slave_state =
+						ASPEED_I2C_SLAVE_READ_REQUESTED;
+			else
+				bus->slave_state =
+						ASPEED_I2C_SLAVE_WRITE_REQUESTED;
+		}
+		status_ack |= ASPEED_I2CD_INTR_RX_DONE;
+	}
+
+	/* Slave was asked to stop. */
+	if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+		status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
+		bus->slave_state = ASPEED_I2C_SLAVE_STOP;
+	}
+	if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
+		status_ack |= ASPEED_I2CD_INTR_TX_NAK;
+		bus->slave_state = ASPEED_I2C_SLAVE_STOP;
+	}
+
+	switch (bus->slave_state) {
+	case ASPEED_I2C_SLAVE_READ_REQUESTED:
+		if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
+			dev_err(bus->dev, "Unexpected ACK on read request.\n");
+		bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
+
+		i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
+		writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
+		writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
+		break;
+	case ASPEED_I2C_SLAVE_READ_PROCESSED:
+		status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+		if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
+			dev_err(bus->dev,
+				"Expected ACK after processed read.\n");
+		i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
+		writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
+		writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
+		break;
+	case ASPEED_I2C_SLAVE_WRITE_REQUESTED:
+		bus->slave_state = ASPEED_I2C_SLAVE_WRITE_RECEIVED;
+		i2c_slave_event(slave, I2C_SLAVE_WRITE_REQUESTED, &value);
+		break;
+	case ASPEED_I2C_SLAVE_WRITE_RECEIVED:
+		i2c_slave_event(slave, I2C_SLAVE_WRITE_RECEIVED, &value);
+		break;
+	case ASPEED_I2C_SLAVE_STOP:
+		i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
+		break;
+	default:
+		dev_err(bus->dev, "unhandled slave_state: %d\n",
+			bus->slave_state);
+		break;
+	}
+
+	if (status_ack != irq_status)
+		dev_err(bus->dev,
+			"irq handled != irq. expected %x, but was %x\n",
+			irq_status, status_ack);
+	writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
+
+out:
+	spin_unlock(&bus->lock);
+	return irq_handled;
+}
+#endif /* CONFIG_I2C_SLAVE */
+
 /* precondition: bus.lock has been acquired. */
 static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
 {
@@ -408,6 +530,13 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
 {
 	struct aspeed_i2c_bus *bus = dev_id;
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	if (aspeed_i2c_slave_irq(bus)) {
+		dev_dbg(bus->dev, "irq handled by slave.\n");
+		return IRQ_HANDLED;
+	}
+#endif /* CONFIG_I2C_SLAVE */
+
 	if (aspeed_i2c_master_irq(bus))
 		return IRQ_HANDLED;
 	else
@@ -466,9 +595,76 @@ static u32 aspeed_i2c_functionality(struct i2c_adapter *adap)
 	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
 }
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+/* precondition: bus.lock has been acquired. */
+static void __aspeed_i2c_reg_slave(struct aspeed_i2c_bus *bus, u16 slave_addr)
+{
+	u32 addr_reg_val, func_ctrl_reg_val;
+
+	/* Set slave addr. */
+	addr_reg_val = readl(bus->base + ASPEED_I2C_DEV_ADDR_REG);
+	addr_reg_val &= ~ASPEED_I2CD_DEV_ADDR_MASK;
+	addr_reg_val |= slave_addr & ASPEED_I2CD_DEV_ADDR_MASK;
+	writel(addr_reg_val, bus->base + ASPEED_I2C_DEV_ADDR_REG);
+
+	/* Turn on slave mode. */
+	func_ctrl_reg_val = readl(bus->base + ASPEED_I2C_FUN_CTRL_REG);
+	func_ctrl_reg_val |= ASPEED_I2CD_SLAVE_EN;
+	writel(func_ctrl_reg_val, bus->base + ASPEED_I2C_FUN_CTRL_REG);
+}
+
+static int aspeed_i2c_reg_slave(struct i2c_client *client)
+{
+	struct aspeed_i2c_bus *bus;
+	unsigned long flags;
+
+	bus = client->adapter->algo_data;
+	spin_lock_irqsave(&bus->lock, flags);
+	if (bus->slave) {
+		spin_unlock_irqrestore(&bus->lock, flags);
+		return -EINVAL;
+	}
+
+	__aspeed_i2c_reg_slave(bus, client->addr);
+
+	bus->slave = client;
+	bus->slave_state = ASPEED_I2C_SLAVE_STOP;
+	spin_unlock_irqrestore(&bus->lock, flags);
+
+	return 0;
+}
+
+static int aspeed_i2c_unreg_slave(struct i2c_client *client)
+{
+	struct aspeed_i2c_bus *bus = client->adapter->algo_data;
+	u32 func_ctrl_reg_val;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bus->lock, flags);
+	if (!bus->slave) {
+		spin_unlock_irqrestore(&bus->lock, flags);
+		return -EINVAL;
+	}
+
+	/* Turn off slave mode. */
+	func_ctrl_reg_val = readl(bus->base + ASPEED_I2C_FUN_CTRL_REG);
+	func_ctrl_reg_val &= ~ASPEED_I2CD_SLAVE_EN;
+	writel(func_ctrl_reg_val, bus->base + ASPEED_I2C_FUN_CTRL_REG);
+
+	bus->slave = NULL;
+	spin_unlock_irqrestore(&bus->lock, flags);
+
+	return 0;
+}
+#endif /* CONFIG_I2C_SLAVE */
+
 static const struct i2c_algorithm aspeed_i2c_algo = {
 	.master_xfer	= aspeed_i2c_master_xfer,
 	.functionality	= aspeed_i2c_functionality,
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	.reg_slave	= aspeed_i2c_reg_slave,
+	.unreg_slave	= aspeed_i2c_unreg_slave,
+#endif /* CONFIG_I2C_SLAVE */
 };
 
 static u32 aspeed_i2c_get_clk_reg_val(u32 divisor)
@@ -561,6 +757,12 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus *bus,
 	writel(readl(bus->base + ASPEED_I2C_FUN_CTRL_REG) | fun_ctrl_reg,
 	       bus->base + ASPEED_I2C_FUN_CTRL_REG);
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	/* If slave has already been registered, re-enable it. */
+	if (bus->slave)
+		__aspeed_i2c_reg_slave(bus, bus->slave->addr);
+#endif /* CONFIG_I2C_SLAVE */
+
 	/* Set interrupt generation of I2C controller */
 	writel(ASPEED_I2CD_INTR_ALL, bus->base + ASPEED_I2C_INTR_CTRL_REG);
 
-- 
2.13.0.219.gdb65acc882-goog

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

[PATCH v8 3/5] i2c: aspeed: added documentation for Aspeed I2C driver

[Brendan Higgins]

Added device tree binding documentation for Aspeed I2C busses.

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
Changes for v2:
  - None
Changes for v3:
  - Removed reference to "bus" device tree param
Changes for v4:
  - None
Changes for v5:
  - None
Changes for v6:
  - Replaced the controller property with and interrupt controller, leaving only
    the busses in the I2C documentation.
Changes for v7:
  - Changed clock-frequency to bus-frequency in device tree
Changes for v8:
  - Added multi-master property to device tree
---
 .../devicetree/bindings/i2c/i2c-aspeed.txt         | 48 ++++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-aspeed.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
new file mode 100644
index 000000000000..bd6480b19535
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
@@ -0,0 +1,48 @@
+Device tree configuration for the I2C busses on the AST24XX and AST25XX SoCs.
+
+Required Properties:
+- #address-cells	: should be 1
+- #size-cells		: should be 0
+- reg			: address offset and range of bus
+- compatible		: should be "aspeed,ast2400-i2c-bus"
+			  or "aspeed,ast2500-i2c-bus"
+- clocks		: root clock of bus, should reference the APB
+			  clock
+- interrupts		: interrupt number
+- interrupt-parent	: interrupt controller for bus, should reference a
+			  aspeed,ast2400-i2c-ic or aspeed,ast2500-i2c-ic
+			  interrupt controller
+
+Optional Properties:
+- bus-frequency	: frequency of the bus clock in Hz defaults to 100 kHz when not
+		  specified
+- multi-master	: states that there is another master active on this bus.
+
+Example:
+
+i2c {
+	compatible = "simple-bus";
+	#address-cells = <1>;
+	#size-cells = <1>;
+	ranges = <0 0x1e78a000 0x1000>;
+
+	i2c_ic: interrupt-controller@0 {
+		#interrupt-cells = <1>;
+		compatible = "aspeed,ast2400-i2c-ic";
+		reg = <0x0 0x40>;
+		interrupts = <12>;
+		interrupt-controller;
+	};
+
+	i2c0: i2c-bus@40 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		#interrupt-cells = <1>;
+		reg = <0x40 0x40>;
+		compatible = "aspeed,ast2400-i2c-bus";
+		clocks = <&clk_apb>;
+		bus-frequency = <100000>;
+		interrupts = <0>;
+		interrupt-parent = <&i2c_ic>;
+	};
+};
-- 
2.13.0.219.gdb65acc882-goog

Re: [PATCH v8 3/5] i2c: aspeed: added documentation for Aspeed I2C driver

[Rob Herring]

On Wed, May 24, 2017 at 11:49:23PM -0700, Brendan Higgins wrote:
> Added device tree binding documentation for Aspeed I2C busses.
> 
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
> Changes for v2:
>   - None
> Changes for v3:
>   - Removed reference to "bus" device tree param
> Changes for v4:
>   - None
> Changes for v5:
>   - None
> Changes for v6:
>   - Replaced the controller property with and interrupt controller, leaving only
>     the busses in the I2C documentation.
> Changes for v7:
>   - Changed clock-frequency to bus-frequency in device tree
> Changes for v8:
>   - Added multi-master property to device tree
> ---
>  .../devicetree/bindings/i2c/i2c-aspeed.txt         | 48 ++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-aspeed.txt

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v8 4/5] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

Added initial master support for Aspeed I2C controller. Supports
fourteen busses present in AST24XX and AST25XX BMC SoCs by Aspeed.

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
Changes for v2:
  - Added single module_init (multiple was breaking some builds).
Changes for v3:
  - Removed "bus" device tree param; now extracted from bus address offset
Changes for v4:
  - I2C adapter number is now generated dynamically unless specified in alias.
Changes for v5:
  - Removed irq_chip used to multiplex IRQ and replaced it with dummy_irq_chip
    along with some other IRQ cleanup.
  - Addressed comments from Cedric, and Vladimir, mostly stylistic things and
    using devm managed resources.
  - Increased max clock frequency before the bus is put in HighSpeed mode, as
    per Kachalov's comment.
Changes for v6:
  - No longer arbitrarily restrict bus to be slave xor master.
  - Pulled out "struct aspeed_i2c_controller" as a interrupt controller.
  - Pulled out slave support into its own commit.
  - Rewrote code that sets clock divider register because the original version
    set it incorrectly.
  - Rewrote the aspeed_i2c_master_irq handler because the old method of
    completing a completion in between restarts was too slow causing devices to
    misbehave.
  - Added support for I2C_M_RECV_LEN which I had incorrectly said was supported
    before.
  - Addressed other comments from Vladimir.
Changes for v7:
  - Changed clock-frequency to bus-frequency
  - Made some fixes to clock divider code
  - Added hardware reset function
  - Marked functions that need to be called with the lock held as "unlocked"
  - Did a bunch of clean up
Changes for v8:
  - ACK IRQ status bits before doing anything else
  - Added multi-master device tree property
  - Do not send STOP commands after interrupt errors
  - Fix SMBUS_QUICK emulation handling
  - Removed highspeed clock code (I will do it in a later patch set)
  - Use the platform_device name for the adapter name
  - Reset for all failed recoveries
  - Removed the "__" prefix from all of the non-thread safe functions
---
 drivers/i2c/busses/Kconfig      |  10 +
 drivers/i2c/busses/Makefile     |   1 +
 drivers/i2c/busses/i2c-aspeed.c | 695 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 706 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-aspeed.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 144cbadc7c72..280f84a0d7d1 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -326,6 +326,16 @@ config I2C_POWERMAC
 
 comment "I2C system bus drivers (mostly embedded / system-on-chip)"
 
+config I2C_ASPEED
+	tristate "Aspeed I2C Controller"
+	depends on ARCH_ASPEED
+	help
+	  If you say yes to this option, support will be included for the
+	  Aspeed I2C controller.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-aspeed.
+
 config I2C_AT91
 	tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
 	depends on ARCH_AT91
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 30b60855fbcd..e84604b9bf3b 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
 obj-$(CONFIG_I2C_POWERMAC)	+= i2c-powermac.o
 
 # Embedded system I2C/SMBus host controller drivers
+obj-$(CONFIG_I2C_ASPEED)	+= i2c-aspeed.o
 obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
 obj-$(CONFIG_I2C_AU1550)	+= i2c-au1550.o
 obj-$(CONFIG_I2C_AXXIA)		+= i2c-axxia.o
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
new file mode 100644
index 000000000000..3eb1252442d7
--- /dev/null
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -0,0 +1,695 @@
+/*
+ *  Aspeed 24XX/25XX I2C Controller.
+ *
+ *  Copyright (C) 2012-2017 ASPEED Technology Inc.
+ *  Copyright 2017 IBM Corporation
+ *  Copyright 2017 Google, Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/* I2C Register */
+#define ASPEED_I2C_FUN_CTRL_REG				0x00
+#define ASPEED_I2C_AC_TIMING_REG1			0x04
+#define ASPEED_I2C_AC_TIMING_REG2			0x08
+#define ASPEED_I2C_INTR_CTRL_REG			0x0c
+#define ASPEED_I2C_INTR_STS_REG				0x10
+#define ASPEED_I2C_CMD_REG				0x14
+#define ASPEED_I2C_DEV_ADDR_REG				0x18
+#define ASPEED_I2C_BYTE_BUF_REG				0x20
+
+/* Global Register Definition */
+/* 0x00 : I2C Interrupt Status Register  */
+/* 0x08 : I2C Interrupt Target Assignment  */
+
+/* Device Register Definition */
+/* 0x00 : I2CD Function Control Register  */
+#define ASPEED_I2CD_MULTI_MASTER_DIS			BIT(15)
+#define ASPEED_I2CD_SDA_DRIVE_1T_EN			BIT(8)
+#define ASPEED_I2CD_M_SDA_DRIVE_1T_EN			BIT(7)
+#define ASPEED_I2CD_M_HIGH_SPEED_EN			BIT(6)
+#define ASPEED_I2CD_MASTER_EN				BIT(0)
+
+/* 0x04 : I2CD Clock and AC Timing Control Register #1 */
+#define ASPEED_I2CD_TIME_SCL_HIGH_SHIFT			16
+#define ASPEED_I2CD_TIME_SCL_HIGH_MASK			GENMASK(19, 16)
+#define ASPEED_I2CD_TIME_SCL_LOW_SHIFT			12
+#define ASPEED_I2CD_TIME_SCL_LOW_MASK			GENMASK(15, 12)
+#define ASPEED_I2CD_TIME_BASE_DIVISOR_MASK		GENMASK(3, 0)
+#define ASPEED_I2CD_TIME_SCL_REG_MAX			GENMASK(3, 0)
+/* 0x08 : I2CD Clock and AC Timing Control Register #2 */
+#define ASPEED_NO_TIMEOUT_CTRL				0
+
+/* 0x0c : I2CD Interrupt Control Register &
+ * 0x10 : I2CD Interrupt Status Register
+ *
+ * These share bit definitions, so use the same values for the enable &
+ * status bits.
+ */
+#define ASPEED_I2CD_INTR_SDA_DL_TIMEOUT			BIT(14)
+#define ASPEED_I2CD_INTR_BUS_RECOVER_DONE		BIT(13)
+#define ASPEED_I2CD_INTR_SCL_TIMEOUT			BIT(6)
+#define ASPEED_I2CD_INTR_ABNORMAL			BIT(5)
+#define ASPEED_I2CD_INTR_NORMAL_STOP			BIT(4)
+#define ASPEED_I2CD_INTR_ARBIT_LOSS			BIT(3)
+#define ASPEED_I2CD_INTR_RX_DONE			BIT(2)
+#define ASPEED_I2CD_INTR_TX_NAK				BIT(1)
+#define ASPEED_I2CD_INTR_TX_ACK				BIT(0)
+#define ASPEED_I2CD_INTR_ERROR						       \
+		(ASPEED_I2CD_INTR_ARBIT_LOSS |				       \
+		 ASPEED_I2CD_INTR_ABNORMAL |				       \
+		 ASPEED_I2CD_INTR_SCL_TIMEOUT |				       \
+		 ASPEED_I2CD_INTR_SDA_DL_TIMEOUT)
+#define ASPEED_I2CD_INTR_ALL						       \
+		(ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |			       \
+		 ASPEED_I2CD_INTR_BUS_RECOVER_DONE |			       \
+		 ASPEED_I2CD_INTR_SCL_TIMEOUT |				       \
+		 ASPEED_I2CD_INTR_ABNORMAL |				       \
+		 ASPEED_I2CD_INTR_NORMAL_STOP |				       \
+		 ASPEED_I2CD_INTR_ARBIT_LOSS |				       \
+		 ASPEED_I2CD_INTR_RX_DONE |				       \
+		 ASPEED_I2CD_INTR_TX_NAK |				       \
+		 ASPEED_I2CD_INTR_TX_ACK)
+
+/* 0x14 : I2CD Command/Status Register   */
+#define ASPEED_I2CD_SCL_LINE_STS			BIT(18)
+#define ASPEED_I2CD_SDA_LINE_STS			BIT(17)
+#define ASPEED_I2CD_BUS_BUSY_STS			BIT(16)
+#define ASPEED_I2CD_BUS_RECOVER_CMD			BIT(11)
+
+/* Command Bit */
+#define ASPEED_I2CD_M_STOP_CMD				BIT(5)
+#define ASPEED_I2CD_M_S_RX_CMD_LAST			BIT(4)
+#define ASPEED_I2CD_M_RX_CMD				BIT(3)
+#define ASPEED_I2CD_S_TX_CMD				BIT(2)
+#define ASPEED_I2CD_M_TX_CMD				BIT(1)
+#define ASPEED_I2CD_M_START_CMD				BIT(0)
+
+enum aspeed_i2c_master_state {
+	ASPEED_I2C_MASTER_START,
+	ASPEED_I2C_MASTER_TX_FIRST,
+	ASPEED_I2C_MASTER_TX,
+	ASPEED_I2C_MASTER_RX_FIRST,
+	ASPEED_I2C_MASTER_RX,
+	ASPEED_I2C_MASTER_STOP,
+	ASPEED_I2C_MASTER_INACTIVE,
+};
+
+struct aspeed_i2c_bus {
+	struct i2c_adapter		adap;
+	struct device			*dev;
+	void __iomem			*base;
+	/* Synchronizes I/O mem access to base. */
+	spinlock_t			lock;
+	struct completion		cmd_complete;
+	int				irq;
+	/* Transaction state. */
+	enum aspeed_i2c_master_state	master_state;
+	struct i2c_msg			*msgs;
+	size_t				buf_index;
+	size_t				msgs_index;
+	size_t				msgs_count;
+	bool				send_stop;
+	int				cmd_err;
+};
+
+static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus);
+
+static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
+{
+	unsigned long time_left, flags;
+	int ret = 0;
+	u32 command;
+
+	spin_lock_irqsave(&bus->lock, flags);
+	command = readl(bus->base + ASPEED_I2C_CMD_REG);
+
+	if (command & ASPEED_I2CD_SDA_LINE_STS) {
+		/* Bus is idle: no recovery needed. */
+		if (command & ASPEED_I2CD_SCL_LINE_STS)
+			goto out;
+		dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
+			command);
+
+		reinit_completion(&bus->cmd_complete);
+		writel(ASPEED_I2CD_M_STOP_CMD, bus->base + ASPEED_I2C_CMD_REG);
+		spin_unlock_irqrestore(&bus->lock, flags);
+
+		time_left = wait_for_completion_timeout(
+				&bus->cmd_complete, bus->adap.timeout);
+
+		spin_lock_irqsave(&bus->lock, flags);
+		if (time_left == 0)
+			goto reset_out;
+		else if (bus->cmd_err)
+			goto reset_out;
+		/* Recovery failed. */
+		else if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
+			   ASPEED_I2CD_SCL_LINE_STS))
+			goto reset_out;
+	/* Bus error. */
+	} else {
+		dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
+			command);
+
+		reinit_completion(&bus->cmd_complete);
+		writel(ASPEED_I2CD_BUS_RECOVER_CMD,
+		       bus->base + ASPEED_I2C_CMD_REG);
+		spin_unlock_irqrestore(&bus->lock, flags);
+
+		time_left = wait_for_completion_timeout(
+				&bus->cmd_complete, bus->adap.timeout);
+
+		spin_lock_irqsave(&bus->lock, flags);
+		if (time_left == 0)
+			goto reset_out;
+		else if (bus->cmd_err)
+			goto reset_out;
+		/* Recovery failed. */
+		else if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
+			   ASPEED_I2CD_SDA_LINE_STS))
+			goto reset_out;
+	}
+
+out:
+	spin_unlock_irqrestore(&bus->lock, flags);
+
+	return ret;
+
+reset_out:
+	spin_unlock_irqrestore(&bus->lock, flags);
+
+	return aspeed_i2c_reset(bus);
+}
+
+/* precondition: bus.lock has been acquired. */
+static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
+{
+	u32 command = ASPEED_I2CD_M_START_CMD | ASPEED_I2CD_M_TX_CMD;
+	struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
+	u8 slave_addr = msg->addr << 1;
+
+	bus->master_state = ASPEED_I2C_MASTER_START;
+	bus->buf_index = 0;
+
+	if (msg->flags & I2C_M_RD) {
+		slave_addr |= 1;
+		command |= ASPEED_I2CD_M_RX_CMD;
+		/* Need to let the hardware know to NACK after RX. */
+		if (msg->len == 1 && !(msg->flags & I2C_M_RECV_LEN))
+			command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
+	}
+
+	writel(slave_addr, bus->base + ASPEED_I2C_BYTE_BUF_REG);
+	writel(command, bus->base + ASPEED_I2C_CMD_REG);
+}
+
+/* precondition: bus.lock has been acquired. */
+static void aspeed_i2c_do_stop(struct aspeed_i2c_bus *bus)
+{
+	bus->master_state = ASPEED_I2C_MASTER_STOP;
+	writel(ASPEED_I2CD_M_STOP_CMD, bus->base + ASPEED_I2C_CMD_REG);
+}
+
+/* precondition: bus.lock has been acquired. */
+static void aspeed_i2c_next_msg_or_stop(struct aspeed_i2c_bus *bus)
+{
+	if (bus->msgs_index + 1 < bus->msgs_count) {
+		bus->msgs_index++;
+		aspeed_i2c_do_start(bus);
+	} else {
+		aspeed_i2c_do_stop(bus);
+	}
+}
+
+static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
+{
+	u32 irq_status, status_ack = 0, command = 0;
+	struct i2c_msg *msg;
+	u8 recv_byte;
+
+	spin_lock(&bus->lock);
+	irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+	/* Ack all interrupt bits. */
+	writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
+
+	if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
+		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+		status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
+		goto out_complete;
+	}
+
+	/*
+	 * We encountered an interrupt that reports an error: the hardware
+	 * should clear the command queue effectively taking us back to the
+	 * INACTIVE state.
+	 */
+	if (irq_status & ASPEED_I2CD_INTR_ERROR) {
+		dev_dbg(bus->dev, "received error interrupt: 0x%08x",
+			irq_status);
+		bus->cmd_err = -EIO;
+		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+		goto out_complete;
+	}
+
+	/* We are in an invalid state; reset bus to a known state. */
+	if (!bus->msgs && bus->master_state != ASPEED_I2C_MASTER_STOP) {
+		dev_err(bus->dev, "bus in unknown state",
+			irq_status);
+		bus->cmd_err = -EIO;
+		aspeed_i2c_do_stop(bus);
+		goto out_no_complete;
+	}
+	msg = &bus->msgs[bus->msgs_index];
+
+	/*
+	 * START is a special case because we still have to handle a subsequent
+	 * TX or RX immediately after we handle it, so we handle it here and
+	 * then update the state and handle the new state below.
+	 */
+	if (bus->master_state == ASPEED_I2C_MASTER_START) {
+		if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
+			dev_dbg(bus->dev,
+				"no slave present at %02x", msg->addr);
+			status_ack |= ASPEED_I2CD_INTR_TX_NAK;
+			goto error_and_stop;
+		}
+		status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+		if (msg->len == 0) { /* SMBUS_QUICK */
+			aspeed_i2c_do_stop(bus);
+			goto out_no_complete;
+		}
+		if (msg->flags & I2C_M_RD)
+			bus->master_state = ASPEED_I2C_MASTER_RX_FIRST;
+		else
+			bus->master_state = ASPEED_I2C_MASTER_TX_FIRST;
+	}
+
+	switch (bus->master_state) {
+	case ASPEED_I2C_MASTER_TX:
+		if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
+			dev_dbg(bus->dev, "slave NACKed TX");
+			status_ack |= ASPEED_I2CD_INTR_TX_NAK;
+			goto error_and_stop;
+		} else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
+			dev_err(bus->dev, "slave failed to ACK TX");
+			goto error_and_stop;
+		}
+		status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+		/* fallthrough intended */
+	case ASPEED_I2C_MASTER_TX_FIRST:
+		if (bus->buf_index < msg->len) {
+			bus->master_state = ASPEED_I2C_MASTER_TX;
+			writel(msg->buf[bus->buf_index++],
+			       bus->base + ASPEED_I2C_BYTE_BUF_REG);
+			writel(ASPEED_I2CD_M_TX_CMD,
+			       bus->base + ASPEED_I2C_CMD_REG);
+		} else {
+			aspeed_i2c_next_msg_or_stop(bus);
+		}
+		goto out_no_complete;
+	case ASPEED_I2C_MASTER_RX_FIRST:
+		/* RX may not have completed yet (only address cycle) */
+		if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE))
+			goto out_no_complete;
+		/* fallthrough intended */
+	case ASPEED_I2C_MASTER_RX:
+		if (unlikely(!(irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
+			dev_err(bus->dev, "master failed to RX");
+			goto error_and_stop;
+		}
+		status_ack |= ASPEED_I2CD_INTR_RX_DONE;
+
+		recv_byte = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
+		msg->buf[bus->buf_index++] = recv_byte;
+
+		if (msg->flags & I2C_M_RECV_LEN) {
+			if (unlikely(recv_byte > I2C_SMBUS_BLOCK_MAX)) {
+				bus->cmd_err = -EPROTO;
+				aspeed_i2c_do_stop(bus);
+				goto out_no_complete;
+			}
+			msg->len = recv_byte +
+					((msg->flags & I2C_CLIENT_PEC) ? 2 : 1);
+			msg->flags &= ~I2C_M_RECV_LEN;
+		}
+
+		if (bus->buf_index < msg->len) {
+			bus->master_state = ASPEED_I2C_MASTER_RX;
+			command = ASPEED_I2CD_M_RX_CMD;
+			if (bus->buf_index + 1 == msg->len)
+				command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
+			writel(command, bus->base + ASPEED_I2C_CMD_REG);
+		} else {
+			aspeed_i2c_next_msg_or_stop(bus);
+		}
+		goto out_no_complete;
+	case ASPEED_I2C_MASTER_STOP:
+		if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
+			dev_err(bus->dev, "master failed to STOP");
+			bus->cmd_err = -EIO;
+			/* Do not STOP as we have already tried. */
+		} else {
+			status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
+		}
+
+		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+		goto out_complete;
+	case ASPEED_I2C_MASTER_INACTIVE:
+		dev_err(bus->dev,
+			"master received interrupt 0x%08x, but is inactive",
+			irq_status);
+		bus->cmd_err = -EIO;
+		/* Do not STOP as we should be inactive. */
+		goto out_complete;
+	default:
+		WARN(1, "unknown master state\n");
+		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+		bus->cmd_err = -EIO;
+		goto out_complete;
+	}
+error_and_stop:
+	bus->cmd_err = -EIO;
+	aspeed_i2c_do_stop(bus);
+	goto out_no_complete;
+out_complete:
+	complete(&bus->cmd_complete);
+out_no_complete:
+	if (irq_status != status_ack)
+		dev_err(bus->dev,
+			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
+			irq_status, status_ack);
+	spin_unlock(&bus->lock);
+	return !!irq_status;
+}
+
+static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
+{
+	struct aspeed_i2c_bus *bus = dev_id;
+
+	if (aspeed_i2c_master_irq(bus))
+		return IRQ_HANDLED;
+	else
+		return IRQ_NONE;
+}
+
+static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
+				  struct i2c_msg *msgs, int num)
+{
+	struct aspeed_i2c_bus *bus = adap->algo_data;
+	unsigned long time_left, flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&bus->lock, flags);
+	bus->cmd_err = 0;
+
+	/* If bus is busy, attempt recovery. We assume a single master
+	 * environment.
+	 */
+	if (readl(bus->base + ASPEED_I2C_CMD_REG) & ASPEED_I2CD_BUS_BUSY_STS) {
+		spin_unlock_irqrestore(&bus->lock, flags);
+		ret = aspeed_i2c_recover_bus(bus);
+		if (ret)
+			return ret;
+		spin_lock_irqsave(&bus->lock, flags);
+	}
+
+	bus->msgs = msgs;
+	bus->msgs_index = 0;
+	bus->msgs_count = num;
+
+	reinit_completion(&bus->cmd_complete);
+	aspeed_i2c_do_start(bus);
+	spin_unlock_irqrestore(&bus->lock, flags);
+
+	time_left = wait_for_completion_timeout(&bus->cmd_complete,
+						bus->adap.timeout);
+
+	spin_lock_irqsave(&bus->lock, flags);
+	bus->msgs = NULL;
+	if (time_left == 0)
+		ret = -ETIMEDOUT;
+	else
+		ret = bus->cmd_err;
+	spin_unlock_irqrestore(&bus->lock, flags);
+
+	/* If nothing went wrong, return number of messages transferred. */
+	if (ret >= 0)
+		return bus->msgs_index + 1;
+	else
+		return ret;
+}
+
+static u32 aspeed_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
+}
+
+static const struct i2c_algorithm aspeed_i2c_algo = {
+	.master_xfer	= aspeed_i2c_master_xfer,
+	.functionality	= aspeed_i2c_functionality,
+};
+
+static u32 aspeed_i2c_get_clk_reg_val(u32 divisor)
+{
+	u32 base_clk, clk_high, clk_low, tmp;
+
+	/*
+	 * The actual clock frequency of SCL is:
+	 *	SCL_freq = APB_freq / (base_freq * (SCL_high + SCL_low))
+	 *		 = APB_freq / divisor
+	 * where base_freq is a programmable clock divider; its value is
+	 *	base_freq = 1 << base_clk
+	 * SCL_high is the number of base_freq clock cycles that SCL stays high
+	 * and SCL_low is the number of base_freq clock cycles that SCL stays
+	 * low for a period of SCL.
+	 * The actual register has a minimum SCL_high and SCL_low minimum of 1;
+	 * thus, they start counting at zero. So
+	 *	SCL_high = clk_high + 1
+	 *	SCL_low	 = clk_low + 1
+	 * Thus,
+	 *	SCL_freq = APB_freq /
+	 *		((1 << base_clk) * (clk_high + 1 + clk_low + 1))
+	 * The documentation recommends clk_high >= 8 and clk_low >= 7 when
+	 * possible; this last constraint gives us the following solution:
+	 */
+	base_clk = divisor > 33 ? ilog2((divisor - 1) / 32) + 1 : 0;
+	tmp = divisor / (1 << base_clk);
+	clk_high = tmp / 2 + tmp % 2;
+	clk_low = tmp - clk_high;
+
+	clk_high -= 1;
+	clk_low -= 1;
+
+	return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
+		& ASPEED_I2CD_TIME_SCL_HIGH_MASK)
+			| ((clk_low << ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
+			   & ASPEED_I2CD_TIME_SCL_LOW_MASK)
+			| (base_clk & ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
+}
+
+/* precondition: bus.lock has been acquired. */
+static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
+				 struct platform_device *pdev)
+{
+	u32 clk_freq, divisor, clk_reg_val;
+	struct clk *pclk;
+	int ret;
+
+	pclk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pclk)) {
+		dev_err(&pdev->dev, "clk_get failed\n");
+		return PTR_ERR(pclk);
+	}
+	ret = of_property_read_u32(pdev->dev.of_node,
+				   "bus-frequency", &clk_freq);
+	if (ret < 0) {
+		dev_err(&pdev->dev,
+			"Could not read bus-frequency property\n");
+		clk_freq = 100000;
+	}
+	divisor = clk_get_rate(pclk) / clk_freq;
+	/* We just need the clock rate, we don't actually use the clk object. */
+	devm_clk_put(&pdev->dev, pclk);
+
+	clk_reg_val = aspeed_i2c_get_clk_reg_val(divisor);
+	writel(clk_reg_val, bus->base + ASPEED_I2C_AC_TIMING_REG1);
+	writel(ASPEED_NO_TIMEOUT_CTRL, bus->base + ASPEED_I2C_AC_TIMING_REG2);
+
+	return 0;
+}
+
+/* precondition: bus.lock has been acquired. */
+static int aspeed_i2c_init(struct aspeed_i2c_bus *bus,
+			     struct platform_device *pdev)
+{
+	u32 fun_ctrl_reg = ASPEED_I2CD_MASTER_EN;
+	int ret;
+
+	/* Disable everything. */
+	writel(0, bus->base + ASPEED_I2C_FUN_CTRL_REG);
+
+	ret = aspeed_i2c_init_clk(bus, pdev);
+	if (ret < 0)
+		return ret;
+
+	if (!of_property_read_bool(pdev->dev.of_node, "multi-master"))
+		fun_ctrl_reg |= ASPEED_I2CD_MULTI_MASTER_DIS;
+
+	/* Enable Master Mode */
+	writel(readl(bus->base + ASPEED_I2C_FUN_CTRL_REG) | fun_ctrl_reg,
+	       bus->base + ASPEED_I2C_FUN_CTRL_REG);
+
+	/* Set interrupt generation of I2C controller */
+	writel(ASPEED_I2CD_INTR_ALL, bus->base + ASPEED_I2C_INTR_CTRL_REG);
+
+	return 0;
+}
+
+static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)
+{
+	struct platform_device *pdev = to_platform_device(bus->dev);
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&bus->lock, flags);
+
+	/* Disable and quiesce interrupts. */
+	reinit_completion(&bus->cmd_complete);
+	writel(0, bus->base + ASPEED_I2C_INTR_CTRL_REG);
+
+	spin_unlock_irqrestore(&bus->lock, flags);
+	/*
+	 * We need to make sure that there are no interrupts that fired just
+	 * before we grabbed the lock; if that did not happen, then we are going
+	 * to timeout and that is okay.
+	 */
+	wait_for_completion_timeout(&bus->cmd_complete, bus->adap.timeout);
+	spin_lock_irqsave(&bus->lock, flags);
+
+	ret = aspeed_i2c_init(bus, pdev);
+
+	spin_unlock_irqrestore(&bus->lock, flags);
+
+	return ret;
+}
+
+static int aspeed_i2c_probe_bus(struct platform_device *pdev)
+{
+	struct aspeed_i2c_bus *bus;
+	struct resource *res;
+	int ret;
+
+	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
+	if (!bus)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	bus->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(bus->base))
+		return PTR_ERR(bus->base);
+
+	/* Initialize the I2C adapter */
+	spin_lock_init(&bus->lock);
+	init_completion(&bus->cmd_complete);
+	bus->adap.owner = THIS_MODULE;
+	bus->adap.retries = 0;
+	bus->adap.timeout = 5 * HZ;
+	bus->adap.algo = &aspeed_i2c_algo;
+	bus->adap.algo_data = bus;
+	bus->adap.dev.parent = &pdev->dev;
+	bus->adap.dev.of_node = pdev->dev.of_node;
+	strlcpy(bus->adap.name, pdev->name, sizeof(bus->adap.name));
+
+	bus->dev = &pdev->dev;
+
+	/*
+	 * No need to quiesce interrupts because there is no interrupt handler
+	 * installed.
+	 */
+	writel(0, bus->base + ASPEED_I2C_INTR_CTRL_REG);
+	/*
+	 * bus.lock does not need to be held because the interrupt handler has
+	 * not been enabled yet.
+	 */
+	ret = aspeed_i2c_init(bus, pdev);
+	if (ret < 0)
+		return ret;
+
+	bus->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+	ret = devm_request_irq(&pdev->dev, bus->irq, aspeed_i2c_bus_irq,
+			       0, dev_name(&pdev->dev), bus);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_add_adapter(&bus->adap);
+	if (ret < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, bus);
+
+	dev_info(bus->dev, "i2c bus %d registered, irq %d\n",
+		 bus->adap.nr, bus->irq);
+
+	return 0;
+}
+
+static int aspeed_i2c_remove_bus(struct platform_device *pdev)
+{
+	struct aspeed_i2c_bus *bus = platform_get_drvdata(pdev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bus->lock, flags);
+
+	/* Disable everything. */
+	writel(0, bus->base + ASPEED_I2C_FUN_CTRL_REG);
+	writel(0, bus->base + ASPEED_I2C_INTR_CTRL_REG);
+
+	spin_unlock_irqrestore(&bus->lock, flags);
+
+	i2c_del_adapter(&bus->adap);
+
+	return 0;
+}
+
+static const struct of_device_id aspeed_i2c_bus_of_table[] = {
+	{ .compatible = "aspeed,ast2400-i2c-bus", },
+	{ .compatible = "aspeed,ast2500-i2c-bus", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table);
+
+static struct platform_driver aspeed_i2c_bus_driver = {
+	.probe		= aspeed_i2c_probe_bus,
+	.remove		= aspeed_i2c_remove_bus,
+	.driver		= {
+		.name		= "aspeed-i2c-bus",
+		.of_match_table	= aspeed_i2c_bus_of_table,
+	},
+};
+module_platform_driver(aspeed_i2c_bus_driver);
+
+MODULE_AUTHOR("Brendan Higgins <brendanhiggins@google.com>");
+MODULE_DESCRIPTION("Aspeed I2C Bus Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.13.0.219.gdb65acc882-goog

Re: [PATCH v8 4/5] i2c: aspeed: added driver for Aspeed I2C

[Benjamin Herrenschmidt]

On Wed, 2017-05-24 at 23:49 -0700, Brendan Higgins wrote:
> Added initial master support for Aspeed I2C controller. Supports
> fourteen busses present in AST24XX and AST25XX BMC SoCs by Aspeed.

Hi Brendan !

It's getting there :-) I have a few comments mostly about some corner
cases below.

Cheers,
Ben.

> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
> Changes for v2:
>   - Added single module_init (multiple was breaking some builds).
> Changes for v3:
>   - Removed "bus" device tree param; now extracted from bus address offset
> Changes for v4:
>   - I2C adapter number is now generated dynamically unless specified in alias.
> Changes for v5:
>   - Removed irq_chip used to multiplex IRQ and replaced it with dummy_irq_chip
>     along with some other IRQ cleanup.
>   - Addressed comments from Cedric, and Vladimir, mostly stylistic things and
>     using devm managed resources.
>   - Increased max clock frequency before the bus is put in HighSpeed mode, as
>     per Kachalov's comment.
> Changes for v6:
>   - No longer arbitrarily restrict bus to be slave xor master.
>   - Pulled out "struct aspeed_i2c_controller" as a interrupt controller.
>   - Pulled out slave support into its own commit.
>   - Rewrote code that sets clock divider register because the original version
>     set it incorrectly.
>   - Rewrote the aspeed_i2c_master_irq handler because the old method of
>     completing a completion in between restarts was too slow causing devices to
>     misbehave.
>   - Added support for I2C_M_RECV_LEN which I had incorrectly said was supported
>     before.
>   - Addressed other comments from Vladimir.
> Changes for v7:
>   - Changed clock-frequency to bus-frequency
>   - Made some fixes to clock divider code
>   - Added hardware reset function
>   - Marked functions that need to be called with the lock held as "unlocked"
>   - Did a bunch of clean up
> Changes for v8:
>   - ACK IRQ status bits before doing anything else
>   - Added multi-master device tree property
>   - Do not send STOP commands after interrupt errors
>   - Fix SMBUS_QUICK emulation handling
>   - Removed highspeed clock code (I will do it in a later patch set)
>   - Use the platform_device name for the adapter name
>   - Reset for all failed recoveries
>   - Removed the "__" prefix from all of the non-thread safe functions
> ---
>  drivers/i2c/busses/Kconfig      |  10 +
>  drivers/i2c/busses/Makefile     |   1 +
>  drivers/i2c/busses/i2c-aspeed.c | 695 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 706 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-aspeed.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 144cbadc7c72..280f84a0d7d1 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -326,6 +326,16 @@ config I2C_POWERMAC
>  
>  comment "I2C system bus drivers (mostly embedded / system-on-chip)"
>  
> +config I2C_ASPEED
> +	tristate "Aspeed I2C Controller"
> +	depends on ARCH_ASPEED
> +	help
> +	  If you say yes to this option, support will be included for the
> +	  Aspeed I2C controller.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-aspeed.
> +
>  config I2C_AT91
>  	tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
>  	depends on ARCH_AT91
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 30b60855fbcd..e84604b9bf3b 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
>  obj-$(CONFIG_I2C_POWERMAC)	+= i2c-powermac.o
>  
>  # Embedded system I2C/SMBus host controller drivers
> +obj-$(CONFIG_I2C_ASPEED)	+= i2c-aspeed.o
>  obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
>  obj-$(CONFIG_I2C_AU1550)	+= i2c-au1550.o
>  obj-$(CONFIG_I2C_AXXIA)		+= i2c-axxia.o
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> new file mode 100644
> index 000000000000..3eb1252442d7
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -0,0 +1,695 @@
> +/*
> + *  Aspeed 24XX/25XX I2C Controller.
> + *
> + *  Copyright (C) 2012-2017 ASPEED Technology Inc.
> + *  Copyright 2017 IBM Corporation
> + *  Copyright 2017 Google, Inc.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +/* I2C Register */
> +#define ASPEED_I2C_FUN_CTRL_REG				0x00
> +#define ASPEED_I2C_AC_TIMING_REG1			0x04
> +#define ASPEED_I2C_AC_TIMING_REG2			0x08
> +#define ASPEED_I2C_INTR_CTRL_REG			0x0c
> +#define ASPEED_I2C_INTR_STS_REG				0x10
> +#define ASPEED_I2C_CMD_REG				0x14
> +#define ASPEED_I2C_DEV_ADDR_REG				0x18
> +#define ASPEED_I2C_BYTE_BUF_REG				0x20
> +
> +/* Global Register Definition */
> +/* 0x00 : I2C Interrupt Status Register  */
> +/* 0x08 : I2C Interrupt Target Assignment  */
> +
> +/* Device Register Definition */
> +/* 0x00 : I2CD Function Control Register  */
> +#define ASPEED_I2CD_MULTI_MASTER_DIS			BIT(15)
> +#define ASPEED_I2CD_SDA_DRIVE_1T_EN			BIT(8)
> +#define ASPEED_I2CD_M_SDA_DRIVE_1T_EN			BIT(7)
> +#define ASPEED_I2CD_M_HIGH_SPEED_EN			BIT(6)
> +#define ASPEED_I2CD_MASTER_EN				BIT(0)
> +
> +/* 0x04 : I2CD Clock and AC Timing Control Register #1 */
> +#define ASPEED_I2CD_TIME_SCL_HIGH_SHIFT			16
> +#define ASPEED_I2CD_TIME_SCL_HIGH_MASK			GENMASK(19, 16)
> +#define ASPEED_I2CD_TIME_SCL_LOW_SHIFT			12
> +#define ASPEED_I2CD_TIME_SCL_LOW_MASK			GENMASK(15, 12)
> +#define ASPEED_I2CD_TIME_BASE_DIVISOR_MASK		GENMASK(3, 0)
> +#define ASPEED_I2CD_TIME_SCL_REG_MAX			GENMASK(3, 0)
> +/* 0x08 : I2CD Clock and AC Timing Control Register #2 */
> +#define ASPEED_NO_TIMEOUT_CTRL				0
> +
> +/* 0x0c : I2CD Interrupt Control Register &
> + * 0x10 : I2CD Interrupt Status Register
> + *
> + * These share bit definitions, so use the same values for the enable &
> + * status bits.
> + */
> +#define ASPEED_I2CD_INTR_SDA_DL_TIMEOUT			BIT(14)
> +#define ASPEED_I2CD_INTR_BUS_RECOVER_DONE		BIT(13)
> +#define ASPEED_I2CD_INTR_SCL_TIMEOUT			BIT(6)
> +#define ASPEED_I2CD_INTR_ABNORMAL			BIT(5)
> +#define ASPEED_I2CD_INTR_NORMAL_STOP			BIT(4)
> +#define ASPEED_I2CD_INTR_ARBIT_LOSS			BIT(3)
> +#define ASPEED_I2CD_INTR_RX_DONE			BIT(2)
> +#define ASPEED_I2CD_INTR_TX_NAK				BIT(1)
> +#define ASPEED_I2CD_INTR_TX_ACK				BIT(0)
> +#define ASPEED_I2CD_INTR_ERROR						       \
> +		(ASPEED_I2CD_INTR_ARBIT_LOSS |				       \
> +		 ASPEED_I2CD_INTR_ABNORMAL |				       \
> +		 ASPEED_I2CD_INTR_SCL_TIMEOUT |				       \
> +		 ASPEED_I2CD_INTR_SDA_DL_TIMEOUT)
> +#define ASPEED_I2CD_INTR_ALL						       \
> +		(ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |			       \
> +		 ASPEED_I2CD_INTR_BUS_RECOVER_DONE |			       \
> +		 ASPEED_I2CD_INTR_SCL_TIMEOUT |				       \
> +		 ASPEED_I2CD_INTR_ABNORMAL |				       \
> +		 ASPEED_I2CD_INTR_NORMAL_STOP |				       \
> +		 ASPEED_I2CD_INTR_ARBIT_LOSS |				       \
> +		 ASPEED_I2CD_INTR_RX_DONE |				       \
> +		 ASPEED_I2CD_INTR_TX_NAK |				       \
> +		 ASPEED_I2CD_INTR_TX_ACK)
> +
> +/* 0x14 : I2CD Command/Status Register   */
> +#define ASPEED_I2CD_SCL_LINE_STS			BIT(18)
> +#define ASPEED_I2CD_SDA_LINE_STS			BIT(17)
> +#define ASPEED_I2CD_BUS_BUSY_STS			BIT(16)
> +#define ASPEED_I2CD_BUS_RECOVER_CMD			BIT(11)
> +
> +/* Command Bit */
> +#define ASPEED_I2CD_M_STOP_CMD				BIT(5)
> +#define ASPEED_I2CD_M_S_RX_CMD_LAST			BIT(4)
> +#define ASPEED_I2CD_M_RX_CMD				BIT(3)
> +#define ASPEED_I2CD_S_TX_CMD				BIT(2)
> +#define ASPEED_I2CD_M_TX_CMD				BIT(1)
> +#define ASPEED_I2CD_M_START_CMD				BIT(0)
> +
> +enum aspeed_i2c_master_state {
> +	ASPEED_I2C_MASTER_START,
> +	ASPEED_I2C_MASTER_TX_FIRST,
> +	ASPEED_I2C_MASTER_TX,
> +	ASPEED_I2C_MASTER_RX_FIRST,
> +	ASPEED_I2C_MASTER_RX,
> +	ASPEED_I2C_MASTER_STOP,
> +	ASPEED_I2C_MASTER_INACTIVE,
> +};
> +
> +struct aspeed_i2c_bus {
> +	struct i2c_adapter		adap;
> +	struct device			*dev;
> +	void __iomem			*base;
> +	/* Synchronizes I/O mem access to base. */
> +	spinlock_t			lock;
> +	struct completion		cmd_complete;
> +	int				irq;
> +	/* Transaction state. */
> +	enum aspeed_i2c_master_state	master_state;
> +	struct i2c_msg			*msgs;
> +	size_t				buf_index;
> +	size_t				msgs_index;
> +	size_t				msgs_count;
> +	bool				send_stop;
> +	int				cmd_err;
> +};
> +
> +static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus);
> +
> +static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
> +{
> +	unsigned long time_left, flags;
> +	int ret = 0;
> +	u32 command;
> +
> +	spin_lock_irqsave(&bus->lock, flags);
> +	command = readl(bus->base + ASPEED_I2C_CMD_REG);
> +
> +	if (command & ASPEED_I2CD_SDA_LINE_STS) {
> +		/* Bus is idle: no recovery needed. */
> +		if (command & ASPEED_I2CD_SCL_LINE_STS)
> +			goto out;
> +		dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
> +			command);
> +
> +		reinit_completion(&bus->cmd_complete);
> +		writel(ASPEED_I2CD_M_STOP_CMD, bus->base + ASPEED_I2C_CMD_REG);
> +		spin_unlock_irqrestore(&bus->lock, flags);
> +
> +		time_left = wait_for_completion_timeout(
> +				&bus->cmd_complete, bus->adap.timeout);
> +
> +		spin_lock_irqsave(&bus->lock, flags);
> +		if (time_left == 0)
> +			goto reset_out;
> +		else if (bus->cmd_err)
> +			goto reset_out;
> +		/* Recovery failed. */
> +		else if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
> +			   ASPEED_I2CD_SCL_LINE_STS))
> +			goto reset_out;
> +	/* Bus error. */
> +	} else {
> +		dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
> +			command);
> +
> +		reinit_completion(&bus->cmd_complete);
> +		writel(ASPEED_I2CD_BUS_RECOVER_CMD,
> +		       bus->base + ASPEED_I2C_CMD_REG);
> +		spin_unlock_irqrestore(&bus->lock, flags);
> +
> +		time_left = wait_for_completion_timeout(
> +				&bus->cmd_complete, bus->adap.timeout);
> +
> +		spin_lock_irqsave(&bus->lock, flags);
> +		if (time_left == 0)
> +			goto reset_out;
> +		else if (bus->cmd_err)
> +			goto reset_out;
> +		/* Recovery failed. */
> +		else if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
> +			   ASPEED_I2CD_SDA_LINE_STS))
> +			goto reset_out;
> +	}
> +
> +out:
> +	spin_unlock_irqrestore(&bus->lock, flags);
> +
> +	return ret;
> +
> +reset_out:
> +	spin_unlock_irqrestore(&bus->lock, flags);
> +
> +	return aspeed_i2c_reset(bus);
> +}
> +
> +/* precondition: bus.lock has been acquired. */
> +static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
> +{
> +	u32 command = ASPEED_I2CD_M_START_CMD | ASPEED_I2CD_M_TX_CMD;
> +	struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
> +	u8 slave_addr = msg->addr << 1;
> +
> +	bus->master_state = ASPEED_I2C_MASTER_START;
> +	bus->buf_index = 0;
> +
> +	if (msg->flags & I2C_M_RD) {
> +		slave_addr |= 1;
> +		command |= ASPEED_I2CD_M_RX_CMD;
> +		/* Need to let the hardware know to NACK after RX. */
> +		if (msg->len == 1 && !(msg->flags & I2C_M_RECV_LEN))
> +			command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
> +	}
> +
> +	writel(slave_addr, bus->base + ASPEED_I2C_BYTE_BUF_REG);
> +	writel(command, bus->base + ASPEED_I2C_CMD_REG);
> +}
> +
> +/* precondition: bus.lock has been acquired. */
> +static void aspeed_i2c_do_stop(struct aspeed_i2c_bus *bus)
> +{
> +	bus->master_state = ASPEED_I2C_MASTER_STOP;
> +	writel(ASPEED_I2CD_M_STOP_CMD, bus->base + ASPEED_I2C_CMD_REG);
> +}
> +
> +/* precondition: bus.lock has been acquired. */
> +static void aspeed_i2c_next_msg_or_stop(struct aspeed_i2c_bus *bus)
> +{
> +	if (bus->msgs_index + 1 < bus->msgs_count) {
> +		bus->msgs_index++;
> +		aspeed_i2c_do_start(bus);
> +	} else {
> +		aspeed_i2c_do_stop(bus);
> +	}
> +}
> +
> +static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> +{
> +	u32 irq_status, status_ack = 0, command = 0;
> +	struct i2c_msg *msg;
> +	u8 recv_byte;
> +
> +	spin_lock(&bus->lock);
> +	irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> +	/* Ack all interrupt bits. */
> +	writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
> +
> +	if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
> +		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> +		status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
> +		goto out_complete;
> +	}
> +
> +	/*
> +	 * We encountered an interrupt that reports an error: the hardware
> +	 * should clear the command queue effectively taking us back to the
> +	 * INACTIVE state.
> +	 */
> +	if (irq_status & ASPEED_I2CD_INTR_ERROR) {
> +		dev_dbg(bus->dev, "received error interrupt: 0x%08x",
> +			irq_status);
> +		bus->cmd_err = -EIO;
> +		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> +		goto out_complete;
> +	}
> +
> +	/* We are in an invalid state; reset bus to a known state. */
> +	if (!bus->msgs && bus->master_state != ASPEED_I2C_MASTER_STOP) {
> +		dev_err(bus->dev, "bus in unknown state",
> +			irq_status);
> +		bus->cmd_err = -EIO;
> +		aspeed_i2c_do_stop(bus);
> +		goto out_no_complete;
> +	}
> +	msg = &bus->msgs[bus->msgs_index];
> +
> +	/*
> +	 * START is a special case because we still have to handle a subsequent
> +	 * TX or RX immediately after we handle it, so we handle it here and
> +	 * then update the state and handle the new state below.
> +	 */
> +	if (bus->master_state == ASPEED_I2C_MASTER_START) {
> +		if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> +			dev_dbg(bus->dev,
> +				"no slave present at %02x", msg->addr);

I would make that a dev_devel, so that it doesn't fill up the logs in
dynamic debug mode, this is a rather common occurrence.

> +			status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> +			goto error_and_stop;
> +		}
> +		status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> +		if (msg->len == 0) { /* SMBUS_QUICK */
> +			aspeed_i2c_do_stop(bus);
> +			goto out_no_complete;
> +		}
> +		if (msg->flags & I2C_M_RD)
> +			bus->master_state = ASPEED_I2C_MASTER_RX_FIRST;
> +		else
> +			bus->master_state = ASPEED_I2C_MASTER_TX_FIRST;
> +	}
> +
> +	switch (bus->master_state) {
> +	case ASPEED_I2C_MASTER_TX:
> +		if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
> +			dev_dbg(bus->dev, "slave NACKed TX");
> +			status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> +			goto error_and_stop;
> +		} else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> +			dev_err(bus->dev, "slave failed to ACK TX");
> +			goto error_and_stop;
> +		}
> +		status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> +		/* fallthrough intended */
> +	case ASPEED_I2C_MASTER_TX_FIRST:
> +		if (bus->buf_index < msg->len) {
> +			bus->master_state = ASPEED_I2C_MASTER_TX;
> +			writel(msg->buf[bus->buf_index++],
> +			       bus->base + ASPEED_I2C_BYTE_BUF_REG);
> +			writel(ASPEED_I2CD_M_TX_CMD,
> +			       bus->base + ASPEED_I2C_CMD_REG);
> +		} else {
> +			aspeed_i2c_next_msg_or_stop(bus);
> +		}
> +		goto out_no_complete;
> +	case ASPEED_I2C_MASTER_RX_FIRST:
> +		/* RX may not have completed yet (only address cycle) */
> +		if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE))
> +			goto out_no_complete;
> +		/* fallthrough intended */
> +	case ASPEED_I2C_MASTER_RX:
> +		if (unlikely(!(irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
> +			dev_err(bus->dev, "master failed to RX");
> +			goto error_and_stop;
> +		}
> +		status_ack |= ASPEED_I2CD_INTR_RX_DONE;
> +
> +		recv_byte = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
> +		msg->buf[bus->buf_index++] = recv_byte;
> +
> +		if (msg->flags & I2C_M_RECV_LEN) {
> +			if (unlikely(recv_byte > I2C_SMBUS_BLOCK_MAX)) {
> +				bus->cmd_err = -EPROTO;
> +				aspeed_i2c_do_stop(bus);
> +				goto out_no_complete;
> +			}
> +			msg->len = recv_byte +
> +					((msg->flags & I2C_CLIENT_PEC) ? 2 : 1);
> +			msg->flags &= ~I2C_M_RECV_LEN;
> +		}
> +
> +		if (bus->buf_index < msg->len) {
> +			bus->master_state = ASPEED_I2C_MASTER_RX;
> +			command = ASPEED_I2CD_M_RX_CMD;
> +			if (bus->buf_index + 1 == msg->len)
> +				command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
> +			writel(command, bus->base + ASPEED_I2C_CMD_REG);
> +		} else {
> +			aspeed_i2c_next_msg_or_stop(bus);
> +		}
> +		goto out_no_complete;
> +	case ASPEED_I2C_MASTER_STOP:
> +		if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
> +			dev_err(bus->dev, "master failed to STOP");
> +			bus->cmd_err = -EIO;
> +			/* Do not STOP as we have already tried. */
> +		} else {
> +			status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
> +		}
> +
> +		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> +		goto out_complete;
> +	case ASPEED_I2C_MASTER_INACTIVE:
> +		dev_err(bus->dev,
> +			"master received interrupt 0x%08x, but is inactive",
> +			irq_status);
> +		bus->cmd_err = -EIO;
> +		/* Do not STOP as we should be inactive. */
> +		goto out_complete;
> +	default:
> +		WARN(1, "unknown master state\n");
> +		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> +		bus->cmd_err = -EIO;
> +		goto out_complete;
> +	}
> +error_and_stop:
> +	bus->cmd_err = -EIO;
> +	aspeed_i2c_do_stop(bus);
> +	goto out_no_complete;
> +out_complete:
> +	complete(&bus->cmd_complete);
> +out_no_complete:
> +	if (irq_status != status_ack)
> +		dev_err(bus->dev,
> +			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> +			irq_status, status_ack);
> +	spin_unlock(&bus->lock);
> +	return !!irq_status;
> +}
> +
> +static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> +{
> +	struct aspeed_i2c_bus *bus = dev_id;
> +
> +	if (aspeed_i2c_master_irq(bus))
> +		return IRQ_HANDLED;
> +	else
> +		return IRQ_NONE;
> +}
> +
> +static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
> +				  struct i2c_msg *msgs, int num)
> +{
> +	struct aspeed_i2c_bus *bus = adap->algo_data;
> +	unsigned long time_left, flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&bus->lock, flags);
> +	bus->cmd_err = 0;
> +
> +	/* If bus is busy, attempt recovery. We assume a single master
> +	 * environment.
> +	 */
> +	if (readl(bus->base + ASPEED_I2C_CMD_REG) & ASPEED_I2CD_BUS_BUSY_STS) {
> +		spin_unlock_irqrestore(&bus->lock, flags);
> +		ret = aspeed_i2c_recover_bus(bus);
> +		if (ret)
> +			return ret;
> +		spin_lock_irqsave(&bus->lock, flags);

Should you re-clear cmd_err ? The interrupt handler might trigger
it during recovery no ? Or just move the clearing to below.

> +	}
> +
> +	bus->msgs = msgs;
> +	bus->msgs_index = 0;
> +	bus->msgs_count = num;
> +
> +	reinit_completion(&bus->cmd_complete);
> +	aspeed_i2c_do_start(bus);
> +	spin_unlock_irqrestore(&bus->lock, flags);
> +
> +	time_left = wait_for_completion_timeout(&bus->cmd_complete,
> +						bus->adap.timeout);
> +
> +	spin_lock_irqsave(&bus->lock, flags);
> +	bus->msgs = NULL;
> +	if (time_left == 0)
> +		ret = -ETIMEDOUT;
> +	else
> +		ret = bus->cmd_err;
> +	spin_unlock_irqrestore(&bus->lock, flags);

I would make the interrupt handler self-clear msgs and I would copy
cmd_err to a separate field (or ensure it's only set by the interrupt
handler when msgs is non-NULL, by the completion code).

That way you avoid the above lock which is racy, slave activity could
get in there and trigger an error interrupt clobbering cmd_err for
example no ? Or am I missing something... 

> +	/* If nothing went wrong, return number of messages transferred. */
> +	if (ret >= 0)
> +		return bus->msgs_index + 1;
> +	else
> +		return ret;
> +}
> +
> +static u32 aspeed_i2c_functionality(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
> +}
> +
> +static const struct i2c_algorithm aspeed_i2c_algo = {
> +	.master_xfer	= aspeed_i2c_master_xfer,
> +	.functionality	= aspeed_i2c_functionality,
> +};
> +
> +static u32 aspeed_i2c_get_clk_reg_val(u32 divisor)
> +{
> +	u32 base_clk, clk_high, clk_low, tmp;
> +
> +	/*
> +	 * The actual clock frequency of SCL is:
> +	 *	SCL_freq = APB_freq / (base_freq * (SCL_high + SCL_low))
> +	 *		 = APB_freq / divisor
> +	 * where base_freq is a programmable clock divider; its value is
> +	 *	base_freq = 1 << base_clk
> +	 * SCL_high is the number of base_freq clock cycles that SCL stays high
> +	 * and SCL_low is the number of base_freq clock cycles that SCL stays
> +	 * low for a period of SCL.
> +	 * The actual register has a minimum SCL_high and SCL_low minimum of 1;
> +	 * thus, they start counting at zero. So
> +	 *	SCL_high = clk_high + 1
> +	 *	SCL_low	 = clk_low + 1
> +	 * Thus,
> +	 *	SCL_freq = APB_freq /
> +	 *		((1 << base_clk) * (clk_high + 1 + clk_low + 1))
> +	 * The documentation recommends clk_high >= 8 and clk_low >= 7 when
> +	 * possible; this last constraint gives us the following solution:
> +	 */
> +	base_clk = divisor > 33 ? ilog2((divisor - 1) / 32) + 1 : 0;
> +	tmp = divisor / (1 << base_clk);
> +	clk_high = tmp / 2 + tmp % 2;
> +	clk_low = tmp - clk_high;
> +
> +	clk_high -= 1;
> +	clk_low -= 1;
> +
> +	return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
> +		& ASPEED_I2CD_TIME_SCL_HIGH_MASK)
> +			| ((clk_low << ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
> +			   & ASPEED_I2CD_TIME_SCL_LOW_MASK)
> +			| (base_clk & ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
> +}
> +
> +/* precondition: bus.lock has been acquired. */
> +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
> +				 struct platform_device *pdev)
> +{
> +	u32 clk_freq, divisor, clk_reg_val;
> +	struct clk *pclk;
> +	int ret;
> +
> +	pclk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(pclk)) {
> +		dev_err(&pdev->dev, "clk_get failed\n");
> +		return PTR_ERR(pclk);
> +	}

I disagree with the pre-condition. I don't think it's safe
to call devm_clk_get() with a lock held anyway.

I don't think you really need the lock here, but if you think
you really need to block against concurrent slave accesses
then you may need a mutex rather than a lock.

If you want to keep the lock, then do the clk_get/get_rate
at probe time instead and cache the speed. I would do the same with the
property.

Also you should probably call clk_enable() somewhere for when we add
proper clock control, but that's not urgent, we can do that when we get
to that point in a separate patch...

> +	ret = of_property_read_u32(pdev->dev.of_node,
> +				   "bus-frequency", &clk_freq);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev,
> +			"Could not read bus-frequency property\n");
> +		clk_freq = 100000;
> +	}
> +	divisor = clk_get_rate(pclk) / clk_freq;
> +	/* We just need the clock rate, we don't actually use the clk object. */
> +	devm_clk_put(&pdev->dev, pclk);
> +
> +	clk_reg_val = aspeed_i2c_get_clk_reg_val(divisor);
> +	writel(clk_reg_val, bus->base + ASPEED_I2C_AC_TIMING_REG1);
> +	writel(ASPEED_NO_TIMEOUT_CTRL, bus->base + ASPEED_I2C_AC_TIMING_REG2);
> +
> +	return 0;
> +}
> +
> +/* precondition: bus.lock has been acquired. */
> +static int aspeed_i2c_init(struct aspeed_i2c_bus *bus,
> +			     struct platform_device *pdev)
> +{
> +	u32 fun_ctrl_reg = ASPEED_I2CD_MASTER_EN;
> +	int ret;
> +
> +	/* Disable everything. */
> +	writel(0, bus->base + ASPEED_I2C_FUN_CTRL_REG);
> +
> +	ret = aspeed_i2c_init_clk(bus, pdev);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!of_property_read_bool(pdev->dev.of_node, "multi-master"))
> +		fun_ctrl_reg |= ASPEED_I2CD_MULTI_MASTER_DIS;
> +
> +	/* Enable Master Mode */
> +	writel(readl(bus->base + ASPEED_I2C_FUN_CTRL_REG) | fun_ctrl_reg,
> +	       bus->base + ASPEED_I2C_FUN_CTRL_REG);
> +
> +	/* Set interrupt generation of I2C controller */
> +	writel(ASPEED_I2CD_INTR_ALL, bus->base + ASPEED_I2C_INTR_CTRL_REG);
> +
> +	return 0;
> +}
> +
> +static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)
> +{
> +	struct platform_device *pdev = to_platform_device(bus->dev);
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&bus->lock, flags);
> +
> +	/* Disable and quiesce interrupts. */
> +	reinit_completion(&bus->cmd_complete);
> +	writel(0, bus->base + ASPEED_I2C_INTR_CTRL_REG);

See comments below in probe() about cleaning interrupts. They apply
to this as well.

> +	spin_unlock_irqrestore(&bus->lock, flags);
> +	/*
> +	 * We need to make sure that there are no interrupts that fired just
> +	 * before we grabbed the lock; if that did not happen, then we are going
> +	 * to timeout and that is okay.
> +	 */
> +	wait_for_completion_timeout(&bus->cmd_complete, bus->adap.timeout);
> +	spin_lock_irqsave(&bus->lock, flags);

This isn't necessary. Just clear the interrupt status register, that
way even if an interrupt fired, the handler won't "see" anything and
will return. An occasional such spurious interupt is a non-issue.

> +	ret = aspeed_i2c_init(bus, pdev);
> +
> +	spin_unlock_irqrestore(&bus->lock, flags);
> +
> +	return ret;
> +}
> +
> +static int aspeed_i2c_probe_bus(struct platform_device *pdev)
> +{
> +	struct aspeed_i2c_bus *bus;
> +	struct resource *res;
> +	int ret;
> +
> +	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	bus->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(bus->base))
> +		return PTR_ERR(bus->base);
> +
> +	/* Initialize the I2C adapter */
> +	spin_lock_init(&bus->lock);
> +	init_completion(&bus->cmd_complete);
> +	bus->adap.owner = THIS_MODULE;
> +	bus->adap.retries = 0;
> +	bus->adap.timeout = 5 * HZ;
> +	bus->adap.algo = &aspeed_i2c_algo;
> +	bus->adap.algo_data = bus;
> +	bus->adap.dev.parent = &pdev->dev;
> +	bus->adap.dev.of_node = pdev->dev.of_node;
> +	strlcpy(bus->adap.name, pdev->name, sizeof(bus->adap.name));
> +
> +	bus->dev = &pdev->dev;
> +
> +	/*
> +	 * No need to quiesce interrupts because there is no interrupt handler
> +	 * installed.
> +	 */
> +	writel(0, bus->base + ASPEED_I2C_INTR_CTRL_REG);

There seem to be a mismatch between comment and code ;-) You do need to
quiesce them before you register the handler in case some stale bit
is set causing an interrupt before you are ready to handle them.

I would suggest adding

       writel(0xffffffff, bus->base + ASPEED_I2C_INTR_STS_REG);

As well to "ack" anything that was left pending, ie, remove stale
conditions. (You may want to do that in the reset code as well).

There is no guarantee you won't still take an interrupt, it could
already be on its way to the CPU before it's masked, but at least
the interrupt handler will not see anything in the status reg.

The above also applies to your reset code.

> +	/*
> +	 * bus.lock does not need to be held because the interrupt handler has
> +	 * not been enabled yet.
> +	 */
> +	ret = aspeed_i2c_init(bus, pdev);
> +	if (ret < 0)
> +		return ret;
> +
> +	bus->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> +	ret = devm_request_irq(&pdev->dev, bus->irq, aspeed_i2c_bus_irq,
> +			       0, dev_name(&pdev->dev), bus);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_add_adapter(&bus->adap);
> +	if (ret < 0)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, bus);
> +
> +	dev_info(bus->dev, "i2c bus %d registered, irq %d\n",
> +		 bus->adap.nr, bus->irq);
> +
> +	return 0;
> +}
> +
> +static int aspeed_i2c_remove_bus(struct platform_device *pdev)
> +{
> +	struct aspeed_i2c_bus *bus = platform_get_drvdata(pdev);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bus->lock, flags);
> +
> +	/* Disable everything. */
> +	writel(0, bus->base + ASPEED_I2C_FUN_CTRL_REG);
> +	writel(0, bus->base + ASPEED_I2C_INTR_CTRL_REG);
> +
> +	spin_unlock_irqrestore(&bus->lock, flags);
> +
> +	i2c_del_adapter(&bus->adap);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id aspeed_i2c_bus_of_table[] = {
> +	{ .compatible = "aspeed,ast2400-i2c-bus", },
> +	{ .compatible = "aspeed,ast2500-i2c-bus", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table);
> +
> +static struct platform_driver aspeed_i2c_bus_driver = {
> +	.probe		= aspeed_i2c_probe_bus,
> +	.remove		= aspeed_i2c_remove_bus,
> +	.driver		= {
> +		.name		= "aspeed-i2c-bus",
> +		.of_match_table	= aspeed_i2c_bus_of_table,
> +	},
> +};
> +module_platform_driver(aspeed_i2c_bus_driver);
> +
> +MODULE_AUTHOR("Brendan Higgins <brendanhiggins@google.com>");
> +MODULE_DESCRIPTION("Aspeed I2C Bus Driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v8 4/5] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

I addressed all of your comments in my next revision except the one below.

>> +     time_left = wait_for_completion_timeout(&bus->cmd_complete,
>> +                                             bus->adap.timeout);
>> +
>> +     spin_lock_irqsave(&bus->lock, flags);
>> +     bus->msgs = NULL;
>> +     if (time_left == 0)
>> +             ret = -ETIMEDOUT;
>> +     else
>> +             ret = bus->cmd_err;
>> +     spin_unlock_irqrestore(&bus->lock, flags);
>
> I would make the interrupt handler self-clear msgs and I would copy
> cmd_err to a separate field (or ensure it's only set by the interrupt
> handler when msgs is non-NULL, by the completion code).
>
> That way you avoid the above lock which is racy, slave activity could
> get in there and trigger an error interrupt clobbering cmd_err for
> example no ? Or am I missing something...

The slave handler does not touch these fields, so that should be fine.
The only way I can think
that we could get in this state is if we had some sort of error
interrupt fire after we handled a
STOP or previous error; this should not happen, but I think it is
better to be safe than sorry.
Nevertheless, I do not think it is necessary to do more than what I
have already done because
it would mean the bus is in a pretty bad state anyway. Maybe I should
just drop the locks here.

If you disagree, could you elaborate on what you meant by putting
cmd_err in a separate field?
Did you just mean having one for xfer and one for everything else (like resets)?

>
>> +     /* If nothing went wrong, return number of messages transferred. */
>> +     if (ret >= 0)
>> +             return bus->msgs_index + 1;
>> +     else
>> +             return ret;
>> +}
>> +
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 4/5] i2c: aspeed: added driver for Aspeed I2C

[Benjamin Herrenschmidt]

On Fri, 2017-06-02 at 02:29 -0700, Brendan Higgins wrote:
> > That way you avoid the above lock which is racy, slave activity could
> > get in there and trigger an error interrupt clobbering cmd_err for
> > example no ? Or am I missing something...
> 
> The slave handler does not touch these fields, so that should be fine.
> The only way I can think
> that we could get in this state is if we had some sort of error
> interrupt fire after we handled a
> STOP or previous error; this should not happen, but I think it is
> better to be safe than sorry.
> Nevertheless, I do not think it is necessary to do more than what I
> have already done because
> it would mean the bus is in a pretty bad state anyway. Maybe I should
> just drop the locks here.
> 
> If you disagree, could you elaborate on what you meant by putting
> cmd_err in a separate field?
> Did you just mean having one for xfer and one for everything else (like resets)?

None of that is a big deal and definitely not a blocker for merging,
you can always "improve" things with a latter patch.

I was thinking that the act of "completing" a request could cleaner if
entirely done from the interrupt code, thus clearing bus->msgs and
setting a "final" status code to be returned to the caller.

Now that could be "cmd_err", it's just that today, that is written to
under various circumstances that may or may not related to a command
being in progress and this I worry that with the lock dropping, we
might "lose" the value that actually pertain to the command itself (and
possibly caused it to fail).

This is all quite academic however, as you said, if that happens we are
already probably in a pretty bad shape.

I suspect the only errors that would happen in "normal" circumstances
would be loss of arbitration and nacks, which are I think, already
properly handled, at least from my reading of the code.

Cheers,
Ben.

> > > +     /* If nothing went wrong, return number of messages transferred. */
> > > +     if (ret >= 0)
> > > +             return bus->msgs_index + 1;
> > > +     else
> > > +             return ret;
> > > +}
> > > +
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html