[PATCH 0/2] i2c-mv64xxx: Add I2C Transaction Generator support

[PATCH 0/2] i2c-mv64xxx: Add I2C Transaction Generator support

[Gregory CLEMENT]

Hello,

this patch set adds support for the I2C Transaction Generator which
offloads CPU from managing I2C transfer step by step. This feature is
currently only available on the I2C controller IP embedded in the
Armada XP SoC. That's why usage of this mechanism is optional and
activated through device tree.

The first patch modifies the driver itself and should go through i2c
subsystem.

Due to the recent changes in i2c-mv64xxx.c file caused by Russell
King's fixes, I based this patch on the for-next branch of the i2c git
repository.

The second patch updates the device tree to be able to use this new
feature and should go through mvebu subsystem.

The 2 patches are independents for building or even at runtime, but of
course we need both of them to be able to use the I2C Transaction
Generator on the Armada XP SoC.

Thanks

Gregory CLEMENT (1):
  ARM: dts: mvebu: Add the i2c-bridge capability to the mv64xxx-i2c

Zbigniew Bodek (1):
  i2c-mv64xxx: Add I2C Transaction Generator support

 .../devicetree/bindings/i2c/i2c-mv64xxx.txt        |   6 +
 arch/arm/boot/dts/armada-370-xp.dtsi               |   2 -
 arch/arm/boot/dts/armada-370.dtsi                  |   8 ++
 arch/arm/boot/dts/armada-xp.dtsi                   |  10 ++
 drivers/i2c/busses/i2c-mv64xxx.c                   | 143 ++++++++++++++++++++-
 5 files changed, 163 insertions(+), 6 deletions(-)

-- 
1.8.1.2

[PATCH 1/2] i2c-mv64xxx: Add I2C Transaction Generator support

[Gregory CLEMENT]

From: Zbigniew Bodek <zbb@semihalf.com>

The I2C Transaction Generator offloads CPU from managing I2C
transfer step by step.

This feature is currently only available on Armada XP, so usage of
this mechanism is activated through device tree.

[gregory.clement at free-electrons.com: Rewrite some part to be applied
on the the code modified by Russell King's fixes]
[gregory.clement at free-electrons.com: Merge and split the commits to
push them to the accurate maintainer i2c and of]]
[gregory.clement at free-electrons.com: Reword the commit log]

Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 143 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 139 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 6356439..5376dc3 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -24,7 +24,7 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 
-/* Register defines */
+/* Register defines (I2C port) */
 #define	MV64XXX_I2C_REG_SLAVE_ADDR			0x00
 #define	MV64XXX_I2C_REG_DATA				0x04
 #define	MV64XXX_I2C_REG_CONTROL				0x08
@@ -59,6 +59,29 @@
 #define	MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK	0xe8
 #define	MV64XXX_I2C_STATUS_NO_STATUS			0xf8
 
+/* Register defines (I2C bridge) */
+#define	MV64XXX_I2C_REG_TX_DATA_LO			0xC0
+#define	MV64XXX_I2C_REG_TX_DATA_HI			0xC4
+#define	MV64XXX_I2C_REG_RX_DATA_LO			0xC8
+#define	MV64XXX_I2C_REG_RX_DATA_HI			0xCC
+#define	MV64XXX_I2C_REG_BRIDGE_CONTROL			0xD0
+#define	MV64XXX_I2C_REG_BRIDGE_STATUS			0xD4
+#define	MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE		0xD8
+#define	MV64XXX_I2C_REG_BRIDGE_INTR_MASK		0xDC
+#define	MV64XXX_I2C_REG_BRIDGE_TIMING			0xE0
+
+/* Bridge Control values */
+#define	MV64XXX_I2C_BRIDGE_CONTROL_WR			0x00000001
+#define	MV64XXX_I2C_BRIDGE_CONTROL_RD			0x00000002
+#define	MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT		2
+#define	MV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT		0x00001000
+#define	MV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT	13
+#define	MV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT	16
+#define	MV64XXX_I2C_BRIDGE_CONTROL_ENABLE		0x00080000
+
+/* Bridge Status values */
+#define	MV64XXX_I2C_BRIDGE_STATUS_ERROR			0x00000001
+
 /* Driver states */
 enum {
 	MV64XXX_I2C_STATE_INVALID,
@@ -110,6 +133,7 @@ struct mv64xxx_i2c_data {
 	spinlock_t		lock;
 	struct i2c_msg		*msg;
 	struct i2c_adapter	adapter;
+	int			bridge_enabled;
 };
 
 static void
@@ -157,6 +181,16 @@ mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
 	writel(0, drv_data->reg_base + MV64XXX_I2C_REG_EXT_SLAVE_ADDR);
 	writel(MV64XXX_I2C_REG_CONTROL_TWSIEN | MV64XXX_I2C_REG_CONTROL_STOP,
 		drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
+
+	if (drv_data->bridge_enabled) {
+		writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
+		writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_TIMING);
+		writel(0, drv_data->reg_base +
+			MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
+		writel(0, drv_data->reg_base +
+			MV64XXX_I2C_REG_BRIDGE_INTR_MASK);
+	}
+
 	drv_data->state = MV64XXX_I2C_STATE_IDLE;
 }
 
@@ -368,6 +402,19 @@ mv64xxx_i2c_intr(int irq, void *dev_id)
 	irqreturn_t	rc = IRQ_NONE;
 
 	spin_lock_irqsave(&drv_data->lock, flags);
+
+	if (drv_data->bridge_enabled) {
+		if (readl(drv_data->reg_base +
+				MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE)) {
+			writel(0, drv_data->reg_base +
+				MV64XXX_I2C_REG_BRIDGE_CONTROL);
+			writel(0, drv_data->reg_base +
+				MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
+			drv_data->block = 0;
+			wake_up(&drv_data->waitq);
+			rc = IRQ_HANDLED;
+		}
+	}
 	while (readl(drv_data->reg_base + MV64XXX_I2C_REG_CONTROL) &
 						MV64XXX_I2C_REG_CONTROL_IFLG) {
 		status = readl(drv_data->reg_base + MV64XXX_I2C_REG_STATUS);
@@ -446,6 +493,81 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
 	return drv_data->rc;
 }
 
+static int
+mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg)
+{
+	unsigned long data_reg_hi = 0;
+	unsigned long data_reg_lo = 0;
+	unsigned long status_reg;
+	unsigned long ctrl_reg;
+	unsigned long flags;
+	unsigned int i;
+
+	/* Only regular transactions can be offloaded */
+	if ((msg->flags & ~(I2C_M_TEN | I2C_M_RD)) != 0)
+		return 1;
+
+	/* Only 1-8 byte transfers can be offloaded */
+	if (msg->len < 1 || msg->len > 8)
+		return 1;
+
+	/* Build transaction */
+	ctrl_reg = MV64XXX_I2C_BRIDGE_CONTROL_ENABLE |
+		   (msg->addr << MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT);
+
+	if ((msg->flags & I2C_M_TEN) != 0)
+		ctrl_reg |=  MV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT;
+
+	if ((msg->flags & I2C_M_RD) == 0) {
+		for (i = 0; i < 4 && i < msg->len; i++)
+			data_reg_lo = data_reg_lo |
+					(msg->buf[i] << ((i & 0x3) * 8));
+
+		for (i = 4; i < 8 && i < msg->len; i++)
+			data_reg_hi = data_reg_hi |
+					(msg->buf[i] << ((i & 0x3) * 8));
+
+		ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_WR |
+		    (msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT;
+	} else {
+		ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_RD |
+		    (msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT;
+	}
+
+	/* Execute transaction */
+	spin_lock_irqsave(&drv_data->lock, flags);
+	drv_data->block = 1;
+	writel(data_reg_lo, drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_LO);
+	writel(data_reg_hi, drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_HI);
+	writel(ctrl_reg, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
+	spin_unlock_irqrestore(&drv_data->lock, flags);
+
+	mv64xxx_i2c_wait_for_completion(drv_data);
+
+	spin_lock_irqsave(&drv_data->lock, flags);
+	status_reg = readl(drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_STATUS);
+	data_reg_lo = readl(drv_data->reg_base + MV64XXX_I2C_REG_RX_DATA_LO);
+	data_reg_hi = readl(drv_data->reg_base + MV64XXX_I2C_REG_RX_DATA_HI);
+	spin_unlock_irqrestore(&drv_data->lock, flags);
+
+	if (status_reg & MV64XXX_I2C_BRIDGE_STATUS_ERROR)
+		return -EIO;
+
+	if ((msg->flags & I2C_M_RD) != 0) {
+		for (i = 0; i < 4 && i < msg->len; i++) {
+			msg->buf[i] = data_reg_lo & 0xFF;
+			data_reg_lo >>= 8;
+		}
+
+		for (i = 4; i < 8 && i < msg->len; i++) {
+			msg->buf[i] = data_reg_hi & 0xFF;
+			data_reg_hi >>= 8;
+		}
+	}
+
+	return 0;
+}
+
 /*
  *****************************************************************************
  *
@@ -463,13 +585,15 @@ static int
 mv64xxx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 {
 	struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
-	int rc, ret = num;
+	int rc = 1, ret = num;
 
 	BUG_ON(drv_data->msgs != NULL);
 	drv_data->msgs = msgs;
 	drv_data->num_msgs = num;
-
-	rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[0], num == 1);
+	if (drv_data->bridge_enabled)
+		rc = mv64xxx_i2c_offload_msg(drv_data, &msgs[0]);
+	if (rc > 0)
+		rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[0], num == 1);
 	if (rc < 0)
 		ret = rc;
 
@@ -528,6 +652,7 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 {
 	u32 bus_freq, tclk;
 	int rc = 0;
+	const char *bridge_status;
 
 	/* CLK is mandatory when using DT to describe the i2c bus. We
 	 * need to know tclk in order to calculate bus clock
@@ -554,6 +679,15 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 	 * So hard code the value to 1 second.
 	 */
 	drv_data->adapter.timeout = HZ;
+
+	/*
+	 * Acquire information on Transaction Generator support.
+	 */
+	bridge_status = of_get_property(np, "", NULL);
+	if (of_property_read_bool(np, "i2c,i2c-bridge"))
+		drv_data->bridge_enabled = 1;
+	else
+		drv_data->bridge_enabled = 0;
 out:
 	return rc;
 #endif
@@ -607,6 +741,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 		drv_data->freq_n = pdata->freq_n;
 		drv_data->irq = platform_get_irq(pd, 0);
 		drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
+		drv_data->bridge_enabled = 0;
 	} else if (pd->dev.of_node) {
 		rc = mv64xxx_of_config(drv_data, pd->dev.of_node);
 		if (rc)
-- 
1.8.1.2

[PATCH 1/2] i2c-mv64xxx: Add I2C Transaction Generator support

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Fri,  7 Jun 2013 17:42:22 +0200, Gregory CLEMENT wrote:

>  /* Driver states */
>  enum {
>  	MV64XXX_I2C_STATE_INVALID,
> @@ -110,6 +133,7 @@ struct mv64xxx_i2c_data {
>  	spinlock_t		lock;
>  	struct i2c_msg		*msg;
>  	struct i2c_adapter	adapter;
> +	int			bridge_enabled;

bool ?

> @@ -528,6 +652,7 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
>  {
>  	u32 bus_freq, tclk;
>  	int rc = 0;
> +	const char *bridge_status;
>  
>  	/* CLK is mandatory when using DT to describe the i2c bus. We
>  	 * need to know tclk in order to calculate bus clock
> @@ -554,6 +679,15 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
>  	 * So hard code the value to 1 second.
>  	 */
>  	drv_data->adapter.timeout = HZ;
> +
> +	/*
> +	 * Acquire information on Transaction Generator support.
> +	 */
> +	bridge_status = of_get_property(np, "", NULL);

It is not clear to me what this new bridge_status local variable is
doing.

> +	if (of_property_read_bool(np, "i2c,i2c-bridge"))
> +		drv_data->bridge_enabled = 1;
> +	else
> +		drv_data->bridge_enabled = 0;

This could be written directly as:

	drv_data->bridge_enabled = of_property_read_bool(np, "i2c,i2c-bridge");

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/2] i2c-mv64xxx: Add I2C Transaction Generator support

[Russell King - ARM Linux]

On Fri, Jun 07, 2013 at 05:42:22PM +0200, Gregory CLEMENT wrote:
> From: Zbigniew Bodek <zbb@semihalf.com>
> 
> The I2C Transaction Generator offloads CPU from managing I2C
> transfer step by step.
> 
> This feature is currently only available on Armada XP, so usage of
> this mechanism is activated through device tree.
> 
> [gregory.clement at free-electrons.com: Rewrite some part to be applied
> on the the code modified by Russell King's fixes]
> [gregory.clement at free-electrons.com: Merge and split the commits to
> push them to the accurate maintainer i2c and of]]
> [gregory.clement at free-electrons.com: Reword the commit log]
> 
> Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  drivers/i2c/busses/i2c-mv64xxx.c | 143 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 139 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index 6356439..5376dc3 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -24,7 +24,7 @@
>  #include <linux/clk.h>
>  #include <linux/err.h>
>  
> -/* Register defines */
> +/* Register defines (I2C port) */
>  #define	MV64XXX_I2C_REG_SLAVE_ADDR			0x00
>  #define	MV64XXX_I2C_REG_DATA				0x04
>  #define	MV64XXX_I2C_REG_CONTROL				0x08
> @@ -59,6 +59,29 @@
>  #define	MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK	0xe8
>  #define	MV64XXX_I2C_STATUS_NO_STATUS			0xf8
>  
> +/* Register defines (I2C bridge) */
> +#define	MV64XXX_I2C_REG_TX_DATA_LO			0xC0
> +#define	MV64XXX_I2C_REG_TX_DATA_HI			0xC4
> +#define	MV64XXX_I2C_REG_RX_DATA_LO			0xC8
> +#define	MV64XXX_I2C_REG_RX_DATA_HI			0xCC
> +#define	MV64XXX_I2C_REG_BRIDGE_CONTROL			0xD0
> +#define	MV64XXX_I2C_REG_BRIDGE_STATUS			0xD4
> +#define	MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE		0xD8
> +#define	MV64XXX_I2C_REG_BRIDGE_INTR_MASK		0xDC
> +#define	MV64XXX_I2C_REG_BRIDGE_TIMING			0xE0
> +
> +/* Bridge Control values */
> +#define	MV64XXX_I2C_BRIDGE_CONTROL_WR			0x00000001
> +#define	MV64XXX_I2C_BRIDGE_CONTROL_RD			0x00000002
> +#define	MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT		2
> +#define	MV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT		0x00001000
> +#define	MV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT	13
> +#define	MV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT	16
> +#define	MV64XXX_I2C_BRIDGE_CONTROL_ENABLE		0x00080000
> +
> +/* Bridge Status values */
> +#define	MV64XXX_I2C_BRIDGE_STATUS_ERROR			0x00000001
> +
>  /* Driver states */
>  enum {
>  	MV64XXX_I2C_STATE_INVALID,
> @@ -110,6 +133,7 @@ struct mv64xxx_i2c_data {
>  	spinlock_t		lock;
>  	struct i2c_msg		*msg;
>  	struct i2c_adapter	adapter;
> +	int			bridge_enabled;
>  };
>  
>  static void
> @@ -157,6 +181,16 @@ mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
>  	writel(0, drv_data->reg_base + MV64XXX_I2C_REG_EXT_SLAVE_ADDR);
>  	writel(MV64XXX_I2C_REG_CONTROL_TWSIEN | MV64XXX_I2C_REG_CONTROL_STOP,
>  		drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
> +
> +	if (drv_data->bridge_enabled) {
> +		writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
> +		writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_TIMING);
> +		writel(0, drv_data->reg_base +
> +			MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
> +		writel(0, drv_data->reg_base +
> +			MV64XXX_I2C_REG_BRIDGE_INTR_MASK);
> +	}
> +
>  	drv_data->state = MV64XXX_I2C_STATE_IDLE;
>  }
>  
> @@ -368,6 +402,19 @@ mv64xxx_i2c_intr(int irq, void *dev_id)
>  	irqreturn_t	rc = IRQ_NONE;
>  
>  	spin_lock_irqsave(&drv_data->lock, flags);
> +
> +	if (drv_data->bridge_enabled) {
> +		if (readl(drv_data->reg_base +
> +				MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE)) {
> +			writel(0, drv_data->reg_base +
> +				MV64XXX_I2C_REG_BRIDGE_CONTROL);
> +			writel(0, drv_data->reg_base +
> +				MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
> +			drv_data->block = 0;
> +			wake_up(&drv_data->waitq);
> +			rc = IRQ_HANDLED;
> +		}
> +	}
>  	while (readl(drv_data->reg_base + MV64XXX_I2C_REG_CONTROL) &
>  						MV64XXX_I2C_REG_CONTROL_IFLG) {
>  		status = readl(drv_data->reg_base + MV64XXX_I2C_REG_STATUS);
> @@ -446,6 +493,81 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
>  	return drv_data->rc;
>  }
>  
> +static int
> +mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg)
> +{
> +	unsigned long data_reg_hi = 0;
> +	unsigned long data_reg_lo = 0;
> +	unsigned long status_reg;
> +	unsigned long ctrl_reg;
> +	unsigned long flags;
> +	unsigned int i;
> +
> +	/* Only regular transactions can be offloaded */
> +	if ((msg->flags & ~(I2C_M_TEN | I2C_M_RD)) != 0)
> +		return 1;
> +
> +	/* Only 1-8 byte transfers can be offloaded */
> +	if (msg->len < 1 || msg->len > 8)
> +		return 1;
> +
> +	/* Build transaction */
> +	ctrl_reg = MV64XXX_I2C_BRIDGE_CONTROL_ENABLE |
> +		   (msg->addr << MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT);
> +
> +	if ((msg->flags & I2C_M_TEN) != 0)
> +		ctrl_reg |=  MV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT;
> +
> +	if ((msg->flags & I2C_M_RD) == 0) {
> +		for (i = 0; i < 4 && i < msg->len; i++)
> +			data_reg_lo = data_reg_lo |
> +					(msg->buf[i] << ((i & 0x3) * 8));
> +
> +		for (i = 4; i < 8 && i < msg->len; i++)
> +			data_reg_hi = data_reg_hi |
> +					(msg->buf[i] << ((i & 0x3) * 8));
> +
> +		ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_WR |
> +		    (msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT;
> +	} else {
> +		ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_RD |
> +		    (msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT;
> +	}
> +
> +	/* Execute transaction */
> +	spin_lock_irqsave(&drv_data->lock, flags);
> +	drv_data->block = 1;
> +	writel(data_reg_lo, drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_LO);
> +	writel(data_reg_hi, drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_HI);

You can use writel_relaxed with the above two writes - you don't need any
synchronization with memory as you aren't doing DMA.

> +	writel(ctrl_reg, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
> +	spin_unlock_irqrestore(&drv_data->lock, flags);

You can also probably dispense with the spinlock too - the interface should
be idle before you start writing to these registers, and nothing should start
happening until after this last write.  Moreover, the I2C core layer will
ensure that there can't be two concurrent transactions.

> +
> +	mv64xxx_i2c_wait_for_completion(drv_data);
> +
> +	spin_lock_irqsave(&drv_data->lock, flags);
> +	status_reg = readl(drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_STATUS);
> +	data_reg_lo = readl(drv_data->reg_base + MV64XXX_I2C_REG_RX_DATA_LO);
> +	data_reg_hi = readl(drv_data->reg_base + MV64XXX_I2C_REG_RX_DATA_HI);
> +	spin_unlock_irqrestore(&drv_data->lock, flags);

And same here - after the completion returns, the interface should now be
idle again, so there should be no concurrency.

> -
> -	rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[0], num == 1);
> +	if (drv_data->bridge_enabled)
> +		rc = mv64xxx_i2c_offload_msg(drv_data, &msgs[0]);

Note that you're only dealing with the first message, so this is buggy.
mv64xxx_i2c_execute_msg() below now deals with the entire transaction
(so all the i2c_msg structures) in one go from interrupt context,
unlike prior to my patch where messages were dealt with one at a time
(and thereby buggily too!)

[PATCH 2/2] ARM: dts: mvebu: Add the i2c-bridge capability to the mv64xxx-i2c

[Gregory CLEMENT]

The mv64xxx-i2c embedded in the Armada XP have a new feature called
i2c-bridge. This commit split the i2c information into armada-370.dtsi
and armada-xp.dtsi. Most of the data remains the same and stay in the
common file Armada-370-xp.dtsi. With this new feature the size of the
registers are bigger for Armada XP and for this SoCs we add a new flag
for the i2c-bridge capability.

The Device Tree binding documentation is updated accordingly.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt |  6 ++++++
 arch/arm/boot/dts/armada-370-xp.dtsi                  |  2 --
 arch/arm/boot/dts/armada-370.dtsi                     |  8 ++++++++
 arch/arm/boot/dts/armada-xp.dtsi                      | 10 ++++++++++
 4 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
index f46d928..8ede3e7 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
@@ -8,6 +8,12 @@ Required properties :
  - interrupts      : The interrupt number
  - clock-frequency : Desired I2C bus clock frequency in Hz.
 
+Optional  properties :
+
+- i2c,i2c-bridge : This flag indicate that the i2c controller have the
+  Transaction Generator support and we want to use it. Not all the
+  mv64xxx controller have this feature.
+
 Examples:
 
 	i2c at 11000 {
diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 550eb77..b6f475c 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -112,7 +112,6 @@
 
 			i2c0: i2c at 11000 {
 				compatible = "marvell,mv64xxx-i2c";
-				reg = <0x11000 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				interrupts = <31>;
@@ -123,7 +122,6 @@
 
 			i2c1: i2c at 11100 {
 				compatible = "marvell,mv64xxx-i2c";
-				reg = <0x11100 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				interrupts = <32>;
diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index aee2b18..39b26d6 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -155,6 +155,14 @@
 				};
 			};
 
+			i2c0: i2c at 11000 {
+				reg = <0x11000 0x20>;
+			};
+
+			i2c1: i2c at 11100 {
+				reg = <0x11100 0x20>;
+			};
+
 			usb at 50000 {
 				clocks = <&coreclk 0>;
 			};
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index 5b902f9..db36bfb 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -134,6 +134,16 @@
 				};
 			};
 
+			i2c0: i2c at 11000 {
+				reg = <0x11000 0x100>;
+				i2c,i2c-bridge;
+			};
+
+			i2c1: i2c at 11100 {
+				reg = <0x11100 0x100>;
+				i2c,i2c-bridge;
+			};
+
 			usb at 50000 {
 				clocks = <&gateclk 18>;
 			};
-- 
1.8.1.2

[PATCH 2/2] ARM: dts: mvebu: Add the i2c-bridge capability to the mv64xxx-i2c

[Maxime Ripard]

Hi Greg,

On Fri, Jun 07, 2013 at 05:42:23PM +0200, Gregory CLEMENT wrote:
> The mv64xxx-i2c embedded in the Armada XP have a new feature called
> i2c-bridge. This commit split the i2c information into armada-370.dtsi
> and armada-xp.dtsi. Most of the data remains the same and stay in the
> common file Armada-370-xp.dtsi. With this new feature the size of the
> registers are bigger for Armada XP and for this SoCs we add a new flag
> for the i2c-bridge capability.
> 
> The Device Tree binding documentation is updated accordingly.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt |  6 ++++++
>  arch/arm/boot/dts/armada-370-xp.dtsi                  |  2 --
>  arch/arm/boot/dts/armada-370.dtsi                     |  8 ++++++++
>  arch/arm/boot/dts/armada-xp.dtsi                      | 10 ++++++++++
>  4 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> index f46d928..8ede3e7 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> @@ -8,6 +8,12 @@ Required properties :
>   - interrupts      : The interrupt number
>   - clock-frequency : Desired I2C bus clock frequency in Hz.
>  
> +Optional  properties :
> +
> +- i2c,i2c-bridge : This flag indicate that the i2c controller have the
> +  Transaction Generator support and we want to use it. Not all the
> +  mv64xxx controller have this feature.

Why not using a different compatible string here then?

Maxime

[PATCH 2/2] ARM: dts: mvebu: Add the i2c-bridge capability to the mv64xxx-i2c

[Jason Cooper]

On Fri, Jun 07, 2013 at 05:42:23PM +0200, Gregory CLEMENT wrote:
> The mv64xxx-i2c embedded in the Armada XP have a new feature called
> i2c-bridge. This commit split the i2c information into armada-370.dtsi
> and armada-xp.dtsi. Most of the data remains the same and stay in the
> common file Armada-370-xp.dtsi. With this new feature the size of the
> registers are bigger for Armada XP and for this SoCs we add a new flag
> for the i2c-bridge capability.
> 
> The Device Tree binding documentation is updated accordingly.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt |  6 ++++++
>  arch/arm/boot/dts/armada-370-xp.dtsi                  |  2 --
>  arch/arm/boot/dts/armada-370.dtsi                     |  8 ++++++++
>  arch/arm/boot/dts/armada-xp.dtsi                      | 10 ++++++++++
>  4 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> index f46d928..8ede3e7 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> @@ -8,6 +8,12 @@ Required properties :
>   - interrupts      : The interrupt number
>   - clock-frequency : Desired I2C bus clock frequency in Hz.
>  
> +Optional  properties :
> +
> +- i2c,i2c-bridge : This flag indicate that the i2c controller have the
> +  Transaction Generator support and we want to use it. Not all the
> +  mv64xxx controller have this feature.

Do you have a list of which controllers definitely do, or definitely
don't?  That would be helpful for folks adding new boards.


> +
>  Examples:

nit. should the example be updated?

thx,

Jason.

[PATCH 2/2] ARM: dts: mvebu: Add the i2c-bridge capability to the mv64xxx-i2c

[Thomas Petazzoni]

Dear Jason Cooper,

On Fri, 7 Jun 2013 14:09:41 -0400, Jason Cooper wrote:

> > +- i2c,i2c-bridge : This flag indicate that the i2c controller have the
> > +  Transaction Generator support and we want to use it. Not all the
> > +  mv64xxx controller have this feature.
> 
> Do you have a list of which controllers definitely do, or definitely
> don't?  That would be helpful for folks adding new boards.

As mentioned in the first commit log:

""
This feature is currently only available on Armada XP, so usage of
this mechanism is activated through device tree.
""

So Armada XP is the only to have that, for now. We can probably assume
future Marvell SoC may also have this feature, but we don't know for
sure.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 2/2] ARM: dts: mvebu: Add the i2c-bridge capability to the mv64xxx-i2c

[Russell King - ARM Linux]

On Fri, Jun 07, 2013 at 05:42:23PM +0200, Gregory CLEMENT wrote:
> The mv64xxx-i2c embedded in the Armada XP have a new feature called
> i2c-bridge. This commit split the i2c information into armada-370.dtsi
> and armada-xp.dtsi. Most of the data remains the same and stay in the
> common file Armada-370-xp.dtsi. With this new feature the size of the
> registers are bigger for Armada XP and for this SoCs we add a new flag
> for the i2c-bridge capability.

Personally, I don't like this "i2c-bridge" flag either, but for a different
reason - i2c-bridge makes it sound like it's doing something it's not.

What it is doing is as you stated in the cover - it's an offload mechanism,
just like if it were possible to use DMA to supply the data to the I2C
interface.

I think Maxime is correct - this should be identified by a variation in
the compatible string, not by a flag in DT.