[PATCH v5 0/3] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

[PATCH v5 0/3] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

[Gregory CLEMENT]

Hello,

This new version just brings the handle of the "marvell,mv78230-i2"
string as a single compatible string. As there were no other remarks
since about 2 months I hope that this version will be able to be
merged in 3.12.

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.

This series also contains a real fix for the I2C controller of the
Armada XP SoC.

The first two patches modify the driver itself and should go through
i2c subsystem.

The last patch updates the device tree to be able to use this new
feature. I kept this patch in the series for having a coherent series,
but it have already been pulled in the mvebu subsystem.

Thanks,

Changelog:

v4-v5:

 - handle of the "marvell,mv78230-i2" string as a single compatible
   string. Even it is strongly discouraged to use "marvell,
   mv78230-i2" alone, it is better to be able to handle it.


v3->v4:

 - reverse the order of the compatible strings, with the most
   specific first

 - rebased on 3.11-rc1

v2->v3:

 - Introduces a new compatible string mv78230-i2c which will be used
   for the fix and for the offload feature which are only present on
   the Armada XP SoCs

 - Removes the unneeded spin_lock_irqsave pointed by Russell King

 - The offload mechanism is now port of the fsm and handle the
   multiple messages.

 - The flag bridge-enabled is renamed to offload_enabled, but the
   register name stills contains the BRIDGE word to match the
   datasheet.

 - Uses writel_relaxed on the place pointed by Russell King

 - Uses the bool type for the flag (pointed by Thomas Petazzoni)

 - Removes useless code (pointed by Thomas Petazzoni)

 - Updates the bindings documentation

v1->v2:
 - Move the flag for the timing issue from global scope to per device
   scope
 - Assignment is no more done in if condition

Gregory CLEMENT (3):
  i2c-mv64xxx: Add I2C Transaction Generator support
  i2c-mv64xxx: Fix timing issue on Armada XP (errata FE-8471889)
  ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

 .../devicetree/bindings/i2c/i2c-mv64xxx.txt        |  13 +-
 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                   | 217 ++++++++++++++++++++-
 5 files changed, 237 insertions(+), 13 deletions(-)

-- 
1.8.1.2

[PATCH v5 0/3] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

[Gregory CLEMENT]

Hello,

This new version just brings the handle of the "marvell,mv78230-i2"
string as a single compatible string. As there were no other remarks
since about 2 months I hope that this version will be able to be
merged in 3.12.

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.

This series also contains a real fix for the I2C controller of the
Armada XP SoC.

The first two patches modify the driver itself and should go through
i2c subsystem.

The last patch updates the device tree to be able to use this new
feature. I kept this patch in the series for having a coherent series,
but it have already been pulled in the mvebu subsystem.

Thanks,

Changelog:

v4-v5:

 - handle of the "marvell,mv78230-i2" string as a single compatible
   string. Even it is strongly discouraged to use "marvell,
   mv78230-i2" alone, it is better to be able to handle it.


v3->v4:

 - reverse the order of the compatible strings, with the most
   specific first

 - rebased on 3.11-rc1

v2->v3:

 - Introduces a new compatible string mv78230-i2c which will be used
   for the fix and for the offload feature which are only present on
   the Armada XP SoCs

 - Removes the unneeded spin_lock_irqsave pointed by Russell King

 - The offload mechanism is now port of the fsm and handle the
   multiple messages.

 - The flag bridge-enabled is renamed to offload_enabled, but the
   register name stills contains the BRIDGE word to match the
   datasheet.

 - Uses writel_relaxed on the place pointed by Russell King

 - Uses the bool type for the flag (pointed by Thomas Petazzoni)

 - Removes useless code (pointed by Thomas Petazzoni)

 - Updates the bindings documentation

v1->v2:
 - Move the flag for the timing issue from global scope to per device
   scope
 - Assignment is no more done in if condition

Gregory CLEMENT (3):
  i2c-mv64xxx: Add I2C Transaction Generator support
  i2c-mv64xxx: Fix timing issue on Armada XP (errata FE-8471889)
  ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

 .../devicetree/bindings/i2c/i2c-mv64xxx.txt        |  13 +-
 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                   | 217 ++++++++++++++++++++-
 5 files changed, 237 insertions(+), 13 deletions(-)

-- 
1.8.1.2

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

[Gregory CLEMENT]

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.

Based on the work of Piotr Ziecik and rewrote to use the new way of
handling multiples i2c messages.

Signed-off-by: Piotr Ziecik <kosmo-nYOzD4b6Jr9Wk0Htik3J/w@public.gmane.org>
Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 208 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 197 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index b1f42bf..406d9c6 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -55,6 +55,32 @@
 #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
+#define	MV64XXX_I2C_STATUS_OFFLOAD_ERROR		0xf0000001
+#define	MV64XXX_I2C_STATUS_OFFLOAD_OK			0xf0000000
+
+
 /* Driver states */
 enum {
 	MV64XXX_I2C_STATE_INVALID,
@@ -71,14 +97,17 @@ enum {
 enum {
 	MV64XXX_I2C_ACTION_INVALID,
 	MV64XXX_I2C_ACTION_CONTINUE,
+	MV64XXX_I2C_ACTION_OFFLOAD_SEND_START,
 	MV64XXX_I2C_ACTION_SEND_START,
 	MV64XXX_I2C_ACTION_SEND_RESTART,
+	MV64XXX_I2C_ACTION_OFFLOAD_RESTART,
 	MV64XXX_I2C_ACTION_SEND_ADDR_1,
 	MV64XXX_I2C_ACTION_SEND_ADDR_2,
 	MV64XXX_I2C_ACTION_SEND_DATA,
 	MV64XXX_I2C_ACTION_RCV_DATA,
 	MV64XXX_I2C_ACTION_RCV_DATA_STOP,
 	MV64XXX_I2C_ACTION_SEND_STOP,
+	MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP,
 };
 
 struct mv64xxx_i2c_regs {
@@ -117,6 +146,7 @@ struct mv64xxx_i2c_data {
 	spinlock_t		lock;
 	struct i2c_msg		*msg;
 	struct i2c_adapter	adapter;
+	bool			offload_enabled;
 };
 
 static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
@@ -165,6 +195,79 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
 	}
 }
 
+static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
+{
+	unsigned long data_reg_hi = 0;
+	unsigned long data_reg_lo = 0;
+	unsigned long ctrl_reg;
+	unsigned int i;
+	struct i2c_msg *msg = drv_data->msgs;
+
+	drv_data->msg = msg;
+	drv_data->byte_posn = 0;
+	drv_data->bytes_left = msg->len;
+	drv_data->aborting = 0;
+	drv_data->rc = 0;
+	/* 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 */
+	writel_relaxed(data_reg_lo,
+		drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_LO);
+	writel_relaxed(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);
+
+	return 0;
+}
+
+static void
+mv64xxx_i2c_update_offload_data(struct i2c_msg *msg, unsigned long data_reg_hi,
+	unsigned long data_reg_lo)
+{
+	int i;
+
+	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;
+		}
+	}
+
+}
 /*
  *****************************************************************************
  *
@@ -177,6 +280,15 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
 static void
 mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
 {
+	if (drv_data->offload_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);
+	}
+
 	writel(0, drv_data->reg_base + drv_data->reg_offsets.soft_reset);
 	writel(MV64XXX_I2C_BAUD_DIV_M(drv_data->freq_m) | MV64XXX_I2C_BAUD_DIV_N(drv_data->freq_n),
 		drv_data->reg_base + drv_data->reg_offsets.clock);
@@ -283,6 +395,16 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
 		drv_data->rc = -ENXIO;
 		break;
 
+	case MV64XXX_I2C_STATUS_OFFLOAD_OK:
+		if (drv_data->send_stop || drv_data->aborting) {
+			drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP;
+			drv_data->state = MV64XXX_I2C_STATE_IDLE;
+		} else {
+			drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_RESTART;
+			drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_RESTART;
+		}
+		break;
+
 	default:
 		dev_err(&drv_data->adapter.dev,
 			"mv64xxx_i2c_fsm: Ctlr Error -- state: 0x%x, "
@@ -298,21 +420,36 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
 static void
 mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 {
+	unsigned long data_reg_hi = 0;
+	unsigned long data_reg_lo = 0;
+
 	switch(drv_data->action) {
+	case MV64XXX_I2C_ACTION_OFFLOAD_RESTART:
+		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);
+		mv64xxx_i2c_update_offload_data(drv_data->msg, data_reg_hi,
+						data_reg_lo);
+		writel(0, drv_data->reg_base +	MV64XXX_I2C_REG_BRIDGE_CONTROL);
+		writel(0, drv_data->reg_base +
+			MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
+		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_RESTART:
 		/* We should only get here if we have further messages */
 		BUG_ON(drv_data->num_msgs == 0);
 
-		drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
-		writel(drv_data->cntl_bits,
-			drv_data->reg_base + drv_data->reg_offsets.control);
-
 		drv_data->msgs++;
 		drv_data->num_msgs--;
+		if (!(drv_data->offload_enabled &&
+				mv64xxx_i2c_offload_msg(drv_data))) {
+			drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
+			writel(drv_data->cntl_bits,
+			drv_data->reg_base + drv_data->reg_offsets.control);
 
-		/* Setup for the next message */
-		mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
-
+			/* Setup for the next message */
+			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+		}
 		/*
 		 * We're never at the start of the message here, and by this
 		 * time it's already too late to do any protocol mangling.
@@ -326,6 +463,12 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 			drv_data->reg_base + drv_data->reg_offsets.control);
 		break;
 
+	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
+		if (mv64xxx_i2c_offload_msg(drv_data) <= 0)
+			break;
+		else
+			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_START:
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
 			drv_data->reg_base + drv_data->reg_offsets.control);
@@ -375,6 +518,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 			"mv64xxx_i2c_do_action: Invalid action: %d\n",
 			drv_data->action);
 		drv_data->rc = -EIO;
+
 		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_STOP:
 		drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
@@ -383,6 +527,20 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 		drv_data->block = 0;
 		wake_up(&drv_data->waitq);
 		break;
+
+	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP:
+		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);
+		mv64xxx_i2c_update_offload_data(drv_data->msg, data_reg_hi,
+						data_reg_lo);
+		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);
+		break;
 	}
 }
 
@@ -395,6 +553,21 @@ mv64xxx_i2c_intr(int irq, void *dev_id)
 	irqreturn_t	rc = IRQ_NONE;
 
 	spin_lock_irqsave(&drv_data->lock, flags);
+
+	if (drv_data->offload_enabled) {
+		while (readl(drv_data->reg_base +
+				MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE)) {
+			int reg_status = readl(drv_data->reg_base +
+					MV64XXX_I2C_REG_BRIDGE_STATUS);
+			if (reg_status & MV64XXX_I2C_BRIDGE_STATUS_ERROR)
+				status = MV64XXX_I2C_STATUS_OFFLOAD_ERROR;
+			else
+				status = MV64XXX_I2C_STATUS_OFFLOAD_OK;
+			mv64xxx_i2c_fsm(drv_data, status);
+			mv64xxx_i2c_do_action(drv_data);
+			rc = IRQ_HANDLED;
+		}
+	}
 	while (readl(drv_data->reg_base + drv_data->reg_offsets.control) &
 						MV64XXX_I2C_REG_CONTROL_IFLG) {
 		status = readl(drv_data->reg_base + drv_data->reg_offsets.status);
@@ -459,11 +632,15 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
 	unsigned long	flags;
 
 	spin_lock_irqsave(&drv_data->lock, flags);
-	mv64xxx_i2c_prepare_for_io(drv_data, msg);
-
-	drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
-	drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+	if (drv_data->offload_enabled) {
+		drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_SEND_START;
+		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+	} else {
+		mv64xxx_i2c_prepare_for_io(drv_data, msg);
 
+		drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+	}
 	drv_data->send_stop = is_last;
 	drv_data->block = 1;
 	mv64xxx_i2c_do_action(drv_data);
@@ -521,6 +698,7 @@ static const struct i2c_algorithm mv64xxx_i2c_algo = {
 static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
 	{ .compatible = "allwinner,sun4i-i2c", .data = &mv64xxx_i2c_regs_sun4i},
 	{ .compatible = "marvell,mv64xxx-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
+	{ .compatible = "marvell,mv78230-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
 	{}
 };
 MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table);
@@ -601,6 +779,13 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 
 	memcpy(&drv_data->reg_offsets, device->data, sizeof(drv_data->reg_offsets));
 
+	/*
+	 * For controllers embedded in new SoCs activate the
+	 * Transaction Generator support.
+	 */
+	if (of_device_is_compatible(np, "marvell,mv78230-i2c"))
+		drv_data->offload_enabled = true;
+
 out:
 	return rc;
 #endif
@@ -654,6 +839,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->offload_enabled = 0;
 		memcpy(&drv_data->reg_offsets, &mv64xxx_i2c_regs_mv64xxx, sizeof(drv_data->reg_offsets));
 	} else if (pd->dev.of_node) {
 		rc = mv64xxx_of_config(drv_data, &pd->dev);
-- 
1.8.1.2

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

[Gregory CLEMENT]

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.

Based on the work of Piotr Ziecik and rewrote to use the new way of
handling multiples i2c messages.

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

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index b1f42bf..406d9c6 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -55,6 +55,32 @@
 #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
+#define	MV64XXX_I2C_STATUS_OFFLOAD_ERROR		0xf0000001
+#define	MV64XXX_I2C_STATUS_OFFLOAD_OK			0xf0000000
+
+
 /* Driver states */
 enum {
 	MV64XXX_I2C_STATE_INVALID,
@@ -71,14 +97,17 @@ enum {
 enum {
 	MV64XXX_I2C_ACTION_INVALID,
 	MV64XXX_I2C_ACTION_CONTINUE,
+	MV64XXX_I2C_ACTION_OFFLOAD_SEND_START,
 	MV64XXX_I2C_ACTION_SEND_START,
 	MV64XXX_I2C_ACTION_SEND_RESTART,
+	MV64XXX_I2C_ACTION_OFFLOAD_RESTART,
 	MV64XXX_I2C_ACTION_SEND_ADDR_1,
 	MV64XXX_I2C_ACTION_SEND_ADDR_2,
 	MV64XXX_I2C_ACTION_SEND_DATA,
 	MV64XXX_I2C_ACTION_RCV_DATA,
 	MV64XXX_I2C_ACTION_RCV_DATA_STOP,
 	MV64XXX_I2C_ACTION_SEND_STOP,
+	MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP,
 };
 
 struct mv64xxx_i2c_regs {
@@ -117,6 +146,7 @@ struct mv64xxx_i2c_data {
 	spinlock_t		lock;
 	struct i2c_msg		*msg;
 	struct i2c_adapter	adapter;
+	bool			offload_enabled;
 };
 
 static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
@@ -165,6 +195,79 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
 	}
 }
 
+static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
+{
+	unsigned long data_reg_hi = 0;
+	unsigned long data_reg_lo = 0;
+	unsigned long ctrl_reg;
+	unsigned int i;
+	struct i2c_msg *msg = drv_data->msgs;
+
+	drv_data->msg = msg;
+	drv_data->byte_posn = 0;
+	drv_data->bytes_left = msg->len;
+	drv_data->aborting = 0;
+	drv_data->rc = 0;
+	/* 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 */
+	writel_relaxed(data_reg_lo,
+		drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_LO);
+	writel_relaxed(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);
+
+	return 0;
+}
+
+static void
+mv64xxx_i2c_update_offload_data(struct i2c_msg *msg, unsigned long data_reg_hi,
+	unsigned long data_reg_lo)
+{
+	int i;
+
+	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;
+		}
+	}
+
+}
 /*
  *****************************************************************************
  *
@@ -177,6 +280,15 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
 static void
 mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
 {
+	if (drv_data->offload_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);
+	}
+
 	writel(0, drv_data->reg_base + drv_data->reg_offsets.soft_reset);
 	writel(MV64XXX_I2C_BAUD_DIV_M(drv_data->freq_m) | MV64XXX_I2C_BAUD_DIV_N(drv_data->freq_n),
 		drv_data->reg_base + drv_data->reg_offsets.clock);
@@ -283,6 +395,16 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
 		drv_data->rc = -ENXIO;
 		break;
 
+	case MV64XXX_I2C_STATUS_OFFLOAD_OK:
+		if (drv_data->send_stop || drv_data->aborting) {
+			drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP;
+			drv_data->state = MV64XXX_I2C_STATE_IDLE;
+		} else {
+			drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_RESTART;
+			drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_RESTART;
+		}
+		break;
+
 	default:
 		dev_err(&drv_data->adapter.dev,
 			"mv64xxx_i2c_fsm: Ctlr Error -- state: 0x%x, "
@@ -298,21 +420,36 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
 static void
 mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 {
+	unsigned long data_reg_hi = 0;
+	unsigned long data_reg_lo = 0;
+
 	switch(drv_data->action) {
+	case MV64XXX_I2C_ACTION_OFFLOAD_RESTART:
+		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);
+		mv64xxx_i2c_update_offload_data(drv_data->msg, data_reg_hi,
+						data_reg_lo);
+		writel(0, drv_data->reg_base +	MV64XXX_I2C_REG_BRIDGE_CONTROL);
+		writel(0, drv_data->reg_base +
+			MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
+		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_RESTART:
 		/* We should only get here if we have further messages */
 		BUG_ON(drv_data->num_msgs == 0);
 
-		drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
-		writel(drv_data->cntl_bits,
-			drv_data->reg_base + drv_data->reg_offsets.control);
-
 		drv_data->msgs++;
 		drv_data->num_msgs--;
+		if (!(drv_data->offload_enabled &&
+				mv64xxx_i2c_offload_msg(drv_data))) {
+			drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
+			writel(drv_data->cntl_bits,
+			drv_data->reg_base + drv_data->reg_offsets.control);
 
-		/* Setup for the next message */
-		mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
-
+			/* Setup for the next message */
+			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+		}
 		/*
 		 * We're never at the start of the message here, and by this
 		 * time it's already too late to do any protocol mangling.
@@ -326,6 +463,12 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 			drv_data->reg_base + drv_data->reg_offsets.control);
 		break;
 
+	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
+		if (mv64xxx_i2c_offload_msg(drv_data) <= 0)
+			break;
+		else
+			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_START:
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
 			drv_data->reg_base + drv_data->reg_offsets.control);
@@ -375,6 +518,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 			"mv64xxx_i2c_do_action: Invalid action: %d\n",
 			drv_data->action);
 		drv_data->rc = -EIO;
+
 		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_STOP:
 		drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
@@ -383,6 +527,20 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 		drv_data->block = 0;
 		wake_up(&drv_data->waitq);
 		break;
+
+	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP:
+		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);
+		mv64xxx_i2c_update_offload_data(drv_data->msg, data_reg_hi,
+						data_reg_lo);
+		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);
+		break;
 	}
 }
 
@@ -395,6 +553,21 @@ mv64xxx_i2c_intr(int irq, void *dev_id)
 	irqreturn_t	rc = IRQ_NONE;
 
 	spin_lock_irqsave(&drv_data->lock, flags);
+
+	if (drv_data->offload_enabled) {
+		while (readl(drv_data->reg_base +
+				MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE)) {
+			int reg_status = readl(drv_data->reg_base +
+					MV64XXX_I2C_REG_BRIDGE_STATUS);
+			if (reg_status & MV64XXX_I2C_BRIDGE_STATUS_ERROR)
+				status = MV64XXX_I2C_STATUS_OFFLOAD_ERROR;
+			else
+				status = MV64XXX_I2C_STATUS_OFFLOAD_OK;
+			mv64xxx_i2c_fsm(drv_data, status);
+			mv64xxx_i2c_do_action(drv_data);
+			rc = IRQ_HANDLED;
+		}
+	}
 	while (readl(drv_data->reg_base + drv_data->reg_offsets.control) &
 						MV64XXX_I2C_REG_CONTROL_IFLG) {
 		status = readl(drv_data->reg_base + drv_data->reg_offsets.status);
@@ -459,11 +632,15 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
 	unsigned long	flags;
 
 	spin_lock_irqsave(&drv_data->lock, flags);
-	mv64xxx_i2c_prepare_for_io(drv_data, msg);
-
-	drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
-	drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+	if (drv_data->offload_enabled) {
+		drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_SEND_START;
+		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+	} else {
+		mv64xxx_i2c_prepare_for_io(drv_data, msg);
 
+		drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+	}
 	drv_data->send_stop = is_last;
 	drv_data->block = 1;
 	mv64xxx_i2c_do_action(drv_data);
@@ -521,6 +698,7 @@ static const struct i2c_algorithm mv64xxx_i2c_algo = {
 static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
 	{ .compatible = "allwinner,sun4i-i2c", .data = &mv64xxx_i2c_regs_sun4i},
 	{ .compatible = "marvell,mv64xxx-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
+	{ .compatible = "marvell,mv78230-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
 	{}
 };
 MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table);
@@ -601,6 +779,13 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 
 	memcpy(&drv_data->reg_offsets, device->data, sizeof(drv_data->reg_offsets));
 
+	/*
+	 * For controllers embedded in new SoCs activate the
+	 * Transaction Generator support.
+	 */
+	if (of_device_is_compatible(np, "marvell,mv78230-i2c"))
+		drv_data->offload_enabled = true;
+
 out:
 	return rc;
 #endif
@@ -654,6 +839,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->offload_enabled = 0;
 		memcpy(&drv_data->reg_offsets, &mv64xxx_i2c_regs_mv64xxx, sizeof(drv_data->reg_offsets));
 	} else if (pd->dev.of_node) {
 		rc = mv64xxx_of_config(drv_data, &pd->dev);
-- 
1.8.1.2

[PATCH v5 2/3] i2c-mv64xxx: Fix timing issue on Armada XP (errata FE-8471889)

[Gregory CLEMENT]

All the Armada XP (mv78230, mv78260 and mv78460) have a silicon issue
in the I2C controller which violate the i2c repeated start
timing. The I2C standard requires a minimum of 4.7us for the repeated
start condition whereas the I2C controller of the Armada XP this time
is 2.9us.

So this patch adds a 5us delay for the start case only if the
the compatible i2c-mv78230 is set.

Based on the initals patches from Zbigniew Bodek

Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Signed-off-by: Zbigniew Bodek <zbb-nYOzD4b6Jr9Wk0Htik3J/w@public.gmane.org>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 406d9c6..a78d7ee 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -24,6 +24,7 @@
 #include <linux/of_i2c.h>
 #include <linux/clk.h>
 #include <linux/err.h>
+#include <linux/delay.h>
 
 #define MV64XXX_I2C_ADDR_ADDR(val)			((val & 0x7f) << 1)
 #define MV64XXX_I2C_BAUD_DIV_N(val)			(val & 0x7)
@@ -147,6 +148,8 @@ struct mv64xxx_i2c_data {
 	struct i2c_msg		*msg;
 	struct i2c_adapter	adapter;
 	bool			offload_enabled;
+/* 5us delay in order to avoid repeated start timing violation */
+	bool			errata_delay;
 };
 
 static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
@@ -450,6 +453,9 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 			/* Setup for the next message */
 			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
 		}
+		if (drv_data->errata_delay)
+			udelay(5);
+
 		/*
 		 * We're never at the start of the message here, and by this
 		 * time it's already too late to do any protocol mangling.
@@ -509,6 +515,9 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
 			drv_data->reg_base + drv_data->reg_offsets.control);
 		drv_data->block = 0;
+		if (drv_data->errata_delay)
+			udelay(5);
+
 		wake_up(&drv_data->waitq);
 		break;
 
@@ -781,10 +790,12 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 
 	/*
 	 * For controllers embedded in new SoCs activate the
-	 * Transaction Generator support.
+	 * Transaction Generator support and the errata fix.
 	 */
-	if (of_device_is_compatible(np, "marvell,mv78230-i2c"))
+	if (of_device_is_compatible(np, "marvell,mv78230-i2c")) {
 		drv_data->offload_enabled = true;
+		drv_data->errata_delay = true;
+	}
 
 out:
 	return rc;
-- 
1.8.1.2

[PATCH v5 2/3] i2c-mv64xxx: Fix timing issue on Armada XP (errata FE-8471889)

[Gregory CLEMENT]

All the Armada XP (mv78230, mv78260 and mv78460) have a silicon issue
in the I2C controller which violate the i2c repeated start
timing. The I2C standard requires a minimum of 4.7us for the repeated
start condition whereas the I2C controller of the Armada XP this time
is 2.9us.

So this patch adds a 5us delay for the start case only if the
the compatible i2c-mv78230 is set.

Based on the initals patches from Zbigniew Bodek

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Signed-off-by: Zbigniew Bodek <zbb@semihalf.com>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 406d9c6..a78d7ee 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -24,6 +24,7 @@
 #include <linux/of_i2c.h>
 #include <linux/clk.h>
 #include <linux/err.h>
+#include <linux/delay.h>
 
 #define MV64XXX_I2C_ADDR_ADDR(val)			((val & 0x7f) << 1)
 #define MV64XXX_I2C_BAUD_DIV_N(val)			(val & 0x7)
@@ -147,6 +148,8 @@ struct mv64xxx_i2c_data {
 	struct i2c_msg		*msg;
 	struct i2c_adapter	adapter;
 	bool			offload_enabled;
+/* 5us delay in order to avoid repeated start timing violation */
+	bool			errata_delay;
 };
 
 static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
@@ -450,6 +453,9 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 			/* Setup for the next message */
 			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
 		}
+		if (drv_data->errata_delay)
+			udelay(5);
+
 		/*
 		 * We're never at the start of the message here, and by this
 		 * time it's already too late to do any protocol mangling.
@@ -509,6 +515,9 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
 			drv_data->reg_base + drv_data->reg_offsets.control);
 		drv_data->block = 0;
+		if (drv_data->errata_delay)
+			udelay(5);
+
 		wake_up(&drv_data->waitq);
 		break;
 
@@ -781,10 +790,12 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 
 	/*
 	 * For controllers embedded in new SoCs activate the
-	 * Transaction Generator support.
+	 * Transaction Generator support and the errata fix.
 	 */
-	if (of_device_is_compatible(np, "marvell,mv78230-i2c"))
+	if (of_device_is_compatible(np, "marvell,mv78230-i2c")) {
 		drv_data->offload_enabled = true;
+		drv_data->errata_delay = true;
+	}
 
 out:
 	return rc;
-- 
1.8.1.2

[PATCH v5 3/3] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Gregory CLEMENT]

The mv64xxx-i2c embedded in the Armada XP have a new feature to
offload i2c transaction. This new version of the IP come also with
some errata. This lead to the introduction to a another compatible
string.

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 the new compatible string is
used.

The Device Tree binding documentation is updated accordingly.

Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 13 ++++++++++++-
 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, 30 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
index a1ee681..c5dd952 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
@@ -4,7 +4,8 @@
 Required properties :
 
  - reg             : Offset and length of the register set for the device
- - compatible      : Should be "marvell,mv64xxx-i2c"
+ - compatible      : Should be "marvell,mv64xxx-i2c" and "marvell,mv7230-i2c"
+for controller which support the I2C Transaction Generator
  - interrupts      : The interrupt number
 
 Optional properties :
@@ -20,3 +21,13 @@ Examples:
 		interrupts = <29>;
 		clock-frequency = <100000>;
 	};
+
+For a controller which support the I2C Transaction Generator:
+
+	i2c@11000 {
+		compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
+		reg = <0x11000 0x100>;
+		compatible = "marvell,mv64xxx-i2c";
+		interrupts = <29>;
+		clock-frequency = <100000>;
+	};
diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 90b1176..d8b24c9 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -121,7 +121,6 @@
 
 			i2c0: i2c@11000 {
 				compatible = "marvell,mv64xxx-i2c";
-				reg = <0x11000 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				interrupts = <31>;
@@ -132,7 +131,6 @@
 
 			i2c1: i2c@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 fa3dfc6..0e2eefa 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -155,6 +155,14 @@
 				};
 			};
 
+			i2c0: i2c@11000 {
+				reg = <0x11000 0x20>;
+			};
+
+			i2c1: i2c@11100 {
+				reg = <0x11100 0x20>;
+			};
+
 			usb@50000 {
 				clocks = <&coreclk 0>;
 			};
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index 416eb94..e1f2547 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -138,6 +138,16 @@
 				};
 			};
 
+			i2c0: i2c@11000 {
+				compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
+				reg = <0x11000 0x100>;
+			};
+
+			i2c1: i2c@11100 {
+				compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
+				reg = <0x11100 0x100>;
+			};
+
 			usb@50000 {
 				clocks = <&gateclk 18>;
 			};
-- 
1.8.1.2

[PATCH v5 3/3] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Gregory CLEMENT]

The mv64xxx-i2c embedded in the Armada XP have a new feature to
offload i2c transaction. This new version of the IP come also with
some errata. This lead to the introduction to a another compatible
string.

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 the new compatible string is
used.

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 | 13 ++++++++++++-
 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, 30 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
index a1ee681..c5dd952 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
@@ -4,7 +4,8 @@
 Required properties :
 
  - reg             : Offset and length of the register set for the device
- - compatible      : Should be "marvell,mv64xxx-i2c"
+ - compatible      : Should be "marvell,mv64xxx-i2c" and "marvell,mv7230-i2c"
+for controller which support the I2C Transaction Generator
  - interrupts      : The interrupt number
 
 Optional properties :
@@ -20,3 +21,13 @@ Examples:
 		interrupts = <29>;
 		clock-frequency = <100000>;
 	};
+
+For a controller which support the I2C Transaction Generator:
+
+	i2c at 11000 {
+		compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
+		reg = <0x11000 0x100>;
+		compatible = "marvell,mv64xxx-i2c";
+		interrupts = <29>;
+		clock-frequency = <100000>;
+	};
diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 90b1176..d8b24c9 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -121,7 +121,6 @@
 
 			i2c0: i2c at 11000 {
 				compatible = "marvell,mv64xxx-i2c";
-				reg = <0x11000 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				interrupts = <31>;
@@ -132,7 +131,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 fa3dfc6..0e2eefa 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 416eb94..e1f2547 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -138,6 +138,16 @@
 				};
 			};
 
+			i2c0: i2c at 11000 {
+				compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
+				reg = <0x11000 0x100>;
+			};
+
+			i2c1: i2c at 11100 {
+				compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
+				reg = <0x11100 0x100>;
+			};
+
 			usb at 50000 {
 				clocks = <&gateclk 18>;
 			};
-- 
1.8.1.2

Re: [PATCH v5 3/3] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Ezequiel Garcia]

(Sending to devicetree)

On Fri, Aug 09, 2013 at 11:05:58AM +0200, Gregory CLEMENT wrote:
> The mv64xxx-i2c embedded in the Armada XP have a new feature to
> offload i2c transaction. This new version of the IP come also with
> some errata. This lead to the introduction to a another compatible
> string.
> 
> 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 the new compatible string is
> used.
> 
> The Device Tree binding documentation is updated accordingly.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 13 ++++++++++++-
>  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, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> index a1ee681..c5dd952 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> @@ -4,7 +4,8 @@
>  Required properties :
>  
>   - reg             : Offset and length of the register set for the device
> - - compatible      : Should be "marvell,mv64xxx-i2c"
> + - compatible      : Should be "marvell,mv64xxx-i2c" and "marvell,mv7230-i2c"
> +for controller which support the I2C Transaction Generator
>   - interrupts      : The interrupt number
>  
>  Optional properties :
> @@ -20,3 +21,13 @@ Examples:
>  		interrupts = <29>;
>  		clock-frequency = <100000>;
>  	};
> +
> +For a controller which support the I2C Transaction Generator:
> +
> +	i2c@11000 {
> +		compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> +		reg = <0x11000 0x100>;
> +		compatible = "marvell,mv64xxx-i2c";
> +		interrupts = <29>;
> +		clock-frequency = <100000>;
> +	};
> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> index 90b1176..d8b24c9 100644
> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> @@ -121,7 +121,6 @@
>  
>  			i2c0: i2c@11000 {
>  				compatible = "marvell,mv64xxx-i2c";
> -				reg = <0x11000 0x20>;
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  				interrupts = <31>;
> @@ -132,7 +131,6 @@
>  
>  			i2c1: i2c@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 fa3dfc6..0e2eefa 100644
> --- a/arch/arm/boot/dts/armada-370.dtsi
> +++ b/arch/arm/boot/dts/armada-370.dtsi
> @@ -155,6 +155,14 @@
>  				};
>  			};
>  
> +			i2c0: i2c@11000 {
> +				reg = <0x11000 0x20>;
> +			};
> +
> +			i2c1: i2c@11100 {
> +				reg = <0x11100 0x20>;
> +			};
> +
>  			usb@50000 {
>  				clocks = <&coreclk 0>;
>  			};
> diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
> index 416eb94..e1f2547 100644
> --- a/arch/arm/boot/dts/armada-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-xp.dtsi
> @@ -138,6 +138,16 @@
>  				};
>  			};
>  
> +			i2c0: i2c@11000 {
> +				compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> +				reg = <0x11000 0x100>;
> +			};
> +
> +			i2c1: i2c@11100 {
> +				compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> +				reg = <0x11100 0x100>;
> +			};
> +
>  			usb@50000 {
>  				clocks = <&gateclk 18>;
>  			};
> -- 
> 1.8.1.2
> 

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v5 3/3] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Ezequiel Garcia]

(Sending to devicetree)

On Fri, Aug 09, 2013 at 11:05:58AM +0200, Gregory CLEMENT wrote:
> The mv64xxx-i2c embedded in the Armada XP have a new feature to
> offload i2c transaction. This new version of the IP come also with
> some errata. This lead to the introduction to a another compatible
> string.
> 
> 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 the new compatible string is
> used.
> 
> 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 | 13 ++++++++++++-
>  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, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> index a1ee681..c5dd952 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> @@ -4,7 +4,8 @@
>  Required properties :
>  
>   - reg             : Offset and length of the register set for the device
> - - compatible      : Should be "marvell,mv64xxx-i2c"
> + - compatible      : Should be "marvell,mv64xxx-i2c" and "marvell,mv7230-i2c"
> +for controller which support the I2C Transaction Generator
>   - interrupts      : The interrupt number
>  
>  Optional properties :
> @@ -20,3 +21,13 @@ Examples:
>  		interrupts = <29>;
>  		clock-frequency = <100000>;
>  	};
> +
> +For a controller which support the I2C Transaction Generator:
> +
> +	i2c at 11000 {
> +		compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> +		reg = <0x11000 0x100>;
> +		compatible = "marvell,mv64xxx-i2c";
> +		interrupts = <29>;
> +		clock-frequency = <100000>;
> +	};
> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> index 90b1176..d8b24c9 100644
> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> @@ -121,7 +121,6 @@
>  
>  			i2c0: i2c at 11000 {
>  				compatible = "marvell,mv64xxx-i2c";
> -				reg = <0x11000 0x20>;
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  				interrupts = <31>;
> @@ -132,7 +131,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 fa3dfc6..0e2eefa 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 416eb94..e1f2547 100644
> --- a/arch/arm/boot/dts/armada-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-xp.dtsi
> @@ -138,6 +138,16 @@
>  				};
>  			};
>  
> +			i2c0: i2c at 11000 {
> +				compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> +				reg = <0x11000 0x100>;
> +			};
> +
> +			i2c1: i2c at 11100 {
> +				compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> +				reg = <0x11100 0x100>;
> +			};
> +
>  			usb at 50000 {
>  				clocks = <&gateclk 18>;
>  			};
> -- 
> 1.8.1.2
> 

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v5 3/3] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Gregory CLEMENT]

On 09/08/2013 11:13, Ezequiel Garcia wrote:
> (Sending to devicetree)

As written in the cover letter this patch was already taken,
so I am not sure you have to send it again on the devicetree
mailing list, they have enough traffic I think.

> 
> On Fri, Aug 09, 2013 at 11:05:58AM +0200, Gregory CLEMENT wrote:
>> The mv64xxx-i2c embedded in the Armada XP have a new feature to
>> offload i2c transaction. This new version of the IP come also with
>> some errata. This lead to the introduction to a another compatible
>> string.
>>
>> 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 the new compatible string is
>> used.
>>
>> The Device Tree binding documentation is updated accordingly.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 13 ++++++++++++-
>>  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, 30 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> index a1ee681..c5dd952 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> @@ -4,7 +4,8 @@
>>  Required properties :
>>  
>>   - reg             : Offset and length of the register set for the device
>> - - compatible      : Should be "marvell,mv64xxx-i2c"
>> + - compatible      : Should be "marvell,mv64xxx-i2c" and "marvell,mv7230-i2c"
>> +for controller which support the I2C Transaction Generator
>>   - interrupts      : The interrupt number
>>  
>>  Optional properties :
>> @@ -20,3 +21,13 @@ Examples:
>>  		interrupts = <29>;
>>  		clock-frequency = <100000>;
>>  	};
>> +
>> +For a controller which support the I2C Transaction Generator:
>> +
>> +	i2c@11000 {
>> +		compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>> +		reg = <0x11000 0x100>;
>> +		compatible = "marvell,mv64xxx-i2c";
>> +		interrupts = <29>;
>> +		clock-frequency = <100000>;
>> +	};
>> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
>> index 90b1176..d8b24c9 100644
>> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
>> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
>> @@ -121,7 +121,6 @@
>>  
>>  			i2c0: i2c@11000 {
>>  				compatible = "marvell,mv64xxx-i2c";
>> -				reg = <0x11000 0x20>;
>>  				#address-cells = <1>;
>>  				#size-cells = <0>;
>>  				interrupts = <31>;
>> @@ -132,7 +131,6 @@
>>  
>>  			i2c1: i2c@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 fa3dfc6..0e2eefa 100644
>> --- a/arch/arm/boot/dts/armada-370.dtsi
>> +++ b/arch/arm/boot/dts/armada-370.dtsi
>> @@ -155,6 +155,14 @@
>>  				};
>>  			};
>>  
>> +			i2c0: i2c@11000 {
>> +				reg = <0x11000 0x20>;
>> +			};
>> +
>> +			i2c1: i2c@11100 {
>> +				reg = <0x11100 0x20>;
>> +			};
>> +
>>  			usb@50000 {
>>  				clocks = <&coreclk 0>;
>>  			};
>> diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
>> index 416eb94..e1f2547 100644
>> --- a/arch/arm/boot/dts/armada-xp.dtsi
>> +++ b/arch/arm/boot/dts/armada-xp.dtsi
>> @@ -138,6 +138,16 @@
>>  				};
>>  			};
>>  
>> +			i2c0: i2c@11000 {
>> +				compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>> +				reg = <0x11000 0x100>;
>> +			};
>> +
>> +			i2c1: i2c@11100 {
>> +				compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>> +				reg = <0x11100 0x100>;
>> +			};
>> +
>>  			usb@50000 {
>>  				clocks = <&gateclk 18>;
>>  			};
>> -- 
>> 1.8.1.2
>>
> 


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

[PATCH v5 3/3] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Gregory CLEMENT]

On 09/08/2013 11:13, Ezequiel Garcia wrote:
> (Sending to devicetree)

As written in the cover letter this patch was already taken,
so I am not sure you have to send it again on the devicetree
mailing list, they have enough traffic I think.

> 
> On Fri, Aug 09, 2013 at 11:05:58AM +0200, Gregory CLEMENT wrote:
>> The mv64xxx-i2c embedded in the Armada XP have a new feature to
>> offload i2c transaction. This new version of the IP come also with
>> some errata. This lead to the introduction to a another compatible
>> string.
>>
>> 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 the new compatible string is
>> used.
>>
>> 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 | 13 ++++++++++++-
>>  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, 30 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> index a1ee681..c5dd952 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> @@ -4,7 +4,8 @@
>>  Required properties :
>>  
>>   - reg             : Offset and length of the register set for the device
>> - - compatible      : Should be "marvell,mv64xxx-i2c"
>> + - compatible      : Should be "marvell,mv64xxx-i2c" and "marvell,mv7230-i2c"
>> +for controller which support the I2C Transaction Generator
>>   - interrupts      : The interrupt number
>>  
>>  Optional properties :
>> @@ -20,3 +21,13 @@ Examples:
>>  		interrupts = <29>;
>>  		clock-frequency = <100000>;
>>  	};
>> +
>> +For a controller which support the I2C Transaction Generator:
>> +
>> +	i2c at 11000 {
>> +		compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>> +		reg = <0x11000 0x100>;
>> +		compatible = "marvell,mv64xxx-i2c";
>> +		interrupts = <29>;
>> +		clock-frequency = <100000>;
>> +	};
>> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
>> index 90b1176..d8b24c9 100644
>> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
>> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
>> @@ -121,7 +121,6 @@
>>  
>>  			i2c0: i2c at 11000 {
>>  				compatible = "marvell,mv64xxx-i2c";
>> -				reg = <0x11000 0x20>;
>>  				#address-cells = <1>;
>>  				#size-cells = <0>;
>>  				interrupts = <31>;
>> @@ -132,7 +131,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 fa3dfc6..0e2eefa 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 416eb94..e1f2547 100644
>> --- a/arch/arm/boot/dts/armada-xp.dtsi
>> +++ b/arch/arm/boot/dts/armada-xp.dtsi
>> @@ -138,6 +138,16 @@
>>  				};
>>  			};
>>  
>> +			i2c0: i2c at 11000 {
>> +				compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>> +				reg = <0x11000 0x100>;
>> +			};
>> +
>> +			i2c1: i2c at 11100 {
>> +				compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>> +				reg = <0x11100 0x100>;
>> +			};
>> +
>>  			usb at 50000 {
>>  				clocks = <&gateclk 18>;
>>  			};
>> -- 
>> 1.8.1.2
>>
> 


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

Re: [PATCH v5 3/3] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Ezequiel Garcia]

On Fri, Aug 09, 2013 at 11:18:27AM +0200, Gregory CLEMENT wrote:
> On 09/08/2013 11:13, Ezequiel Garcia wrote:
> > (Sending to devicetree)
> 
> As written in the cover letter this patch was already taken,
> so I am not sure you have to send it again on the devicetree
> mailing list, they have enough traffic I think.
> 

Ooops, I missed that. Sorry for the noise.

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v5 3/3] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Ezequiel Garcia]

On Fri, Aug 09, 2013 at 11:18:27AM +0200, Gregory CLEMENT wrote:
> On 09/08/2013 11:13, Ezequiel Garcia wrote:
> > (Sending to devicetree)
> 
> As written in the cover letter this patch was already taken,
> so I am not sure you have to send it again on the devicetree
> mailing list, they have enough traffic I think.
> 

Ooops, I missed that. Sorry for the noise.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v5 3/3] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Jason Cooper]

Ezequiel,

On Fri, Aug 09, 2013 at 06:13:37AM -0300, Ezequiel Garcia wrote:
> (Sending to devicetree)

As an FYI?  I already responded to Mark Rutland's request for the
binding doc to be updated (that it was this patch, already merged into
mvebu/dt).  I also asked him if everything looked ok to him and he said
yes.

Is there something else you're looking for with this patch?

confused,

Jason.

> On Fri, Aug 09, 2013 at 11:05:58AM +0200, Gregory CLEMENT wrote:
> > The mv64xxx-i2c embedded in the Armada XP have a new feature to
> > offload i2c transaction. This new version of the IP come also with
> > some errata. This lead to the introduction to a another compatible
> > string.
> > 
> > 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 the new compatible string is
> > used.
> > 
> > The Device Tree binding documentation is updated accordingly.
> > 
> > Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 13 ++++++++++++-
> >  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, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > index a1ee681..c5dd952 100644
> > --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > @@ -4,7 +4,8 @@
> >  Required properties :
> >  
> >   - reg             : Offset and length of the register set for the device
> > - - compatible      : Should be "marvell,mv64xxx-i2c"
> > + - compatible      : Should be "marvell,mv64xxx-i2c" and "marvell,mv7230-i2c"
> > +for controller which support the I2C Transaction Generator
> >   - interrupts      : The interrupt number
> >  
> >  Optional properties :
> > @@ -20,3 +21,13 @@ Examples:
> >  		interrupts = <29>;
> >  		clock-frequency = <100000>;
> >  	};
> > +
> > +For a controller which support the I2C Transaction Generator:
> > +
> > +	i2c@11000 {
> > +		compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> > +		reg = <0x11000 0x100>;
> > +		compatible = "marvell,mv64xxx-i2c";
> > +		interrupts = <29>;
> > +		clock-frequency = <100000>;
> > +	};
> > diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> > index 90b1176..d8b24c9 100644
> > --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> > +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> > @@ -121,7 +121,6 @@
> >  
> >  			i2c0: i2c@11000 {
> >  				compatible = "marvell,mv64xxx-i2c";
> > -				reg = <0x11000 0x20>;
> >  				#address-cells = <1>;
> >  				#size-cells = <0>;
> >  				interrupts = <31>;
> > @@ -132,7 +131,6 @@
> >  
> >  			i2c1: i2c@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 fa3dfc6..0e2eefa 100644
> > --- a/arch/arm/boot/dts/armada-370.dtsi
> > +++ b/arch/arm/boot/dts/armada-370.dtsi
> > @@ -155,6 +155,14 @@
> >  				};
> >  			};
> >  
> > +			i2c0: i2c@11000 {
> > +				reg = <0x11000 0x20>;
> > +			};
> > +
> > +			i2c1: i2c@11100 {
> > +				reg = <0x11100 0x20>;
> > +			};
> > +
> >  			usb@50000 {
> >  				clocks = <&coreclk 0>;
> >  			};
> > diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
> > index 416eb94..e1f2547 100644
> > --- a/arch/arm/boot/dts/armada-xp.dtsi
> > +++ b/arch/arm/boot/dts/armada-xp.dtsi
> > @@ -138,6 +138,16 @@
> >  				};
> >  			};
> >  
> > +			i2c0: i2c@11000 {
> > +				compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> > +				reg = <0x11000 0x100>;
> > +			};
> > +
> > +			i2c1: i2c@11100 {
> > +				compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> > +				reg = <0x11100 0x100>;
> > +			};
> > +
> >  			usb@50000 {
> >  				clocks = <&gateclk 18>;
> >  			};
> > -- 
> > 1.8.1.2
> > 
> 
> -- 
> Ezequiel García, Free Electrons
> Embedded Linux, Kernel and Android Engineering
> http://free-electrons.com
> --
> 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 v5 3/3] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Jason Cooper]

Ezequiel,

On Fri, Aug 09, 2013 at 06:13:37AM -0300, Ezequiel Garcia wrote:
> (Sending to devicetree)

As an FYI?  I already responded to Mark Rutland's request for the
binding doc to be updated (that it was this patch, already merged into
mvebu/dt).  I also asked him if everything looked ok to him and he said
yes.

Is there something else you're looking for with this patch?

confused,

Jason.

> On Fri, Aug 09, 2013 at 11:05:58AM +0200, Gregory CLEMENT wrote:
> > The mv64xxx-i2c embedded in the Armada XP have a new feature to
> > offload i2c transaction. This new version of the IP come also with
> > some errata. This lead to the introduction to a another compatible
> > string.
> > 
> > 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 the new compatible string is
> > used.
> > 
> > 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 | 13 ++++++++++++-
> >  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, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > index a1ee681..c5dd952 100644
> > --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > @@ -4,7 +4,8 @@
> >  Required properties :
> >  
> >   - reg             : Offset and length of the register set for the device
> > - - compatible      : Should be "marvell,mv64xxx-i2c"
> > + - compatible      : Should be "marvell,mv64xxx-i2c" and "marvell,mv7230-i2c"
> > +for controller which support the I2C Transaction Generator
> >   - interrupts      : The interrupt number
> >  
> >  Optional properties :
> > @@ -20,3 +21,13 @@ Examples:
> >  		interrupts = <29>;
> >  		clock-frequency = <100000>;
> >  	};
> > +
> > +For a controller which support the I2C Transaction Generator:
> > +
> > +	i2c at 11000 {
> > +		compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> > +		reg = <0x11000 0x100>;
> > +		compatible = "marvell,mv64xxx-i2c";
> > +		interrupts = <29>;
> > +		clock-frequency = <100000>;
> > +	};
> > diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> > index 90b1176..d8b24c9 100644
> > --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> > +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> > @@ -121,7 +121,6 @@
> >  
> >  			i2c0: i2c at 11000 {
> >  				compatible = "marvell,mv64xxx-i2c";
> > -				reg = <0x11000 0x20>;
> >  				#address-cells = <1>;
> >  				#size-cells = <0>;
> >  				interrupts = <31>;
> > @@ -132,7 +131,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 fa3dfc6..0e2eefa 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 416eb94..e1f2547 100644
> > --- a/arch/arm/boot/dts/armada-xp.dtsi
> > +++ b/arch/arm/boot/dts/armada-xp.dtsi
> > @@ -138,6 +138,16 @@
> >  				};
> >  			};
> >  
> > +			i2c0: i2c at 11000 {
> > +				compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> > +				reg = <0x11000 0x100>;
> > +			};
> > +
> > +			i2c1: i2c at 11100 {
> > +				compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> > +				reg = <0x11100 0x100>;
> > +			};
> > +
> >  			usb at 50000 {
> >  				clocks = <&gateclk 18>;
> >  			};
> > -- 
> > 1.8.1.2
> > 
> 
> -- 
> Ezequiel Garc?a, Free Electrons
> Embedded Linux, Kernel and Android Engineering
> http://free-electrons.com
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/3] i2c-mv64xxx: Add I2C Transaction Generator support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 400 bytes --]


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

Same comment as in the last version: What about be32_to_cpu and friends
instead of the loops (here and later)?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Wolfram Sang]

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

Same comment as in the last version: What about be32_to_cpu and friends
instead of the loops (here and later)?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130815/4b3717e9/attachment.sig>

Re: [PATCH v5 1/3] i2c-mv64xxx: Add I2C Transaction Generator support

[Gregory CLEMENT]

On 15/08/2013 14:13, Wolfram Sang wrote:
> 
>> +	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));
> 
> Same comment as in the last version: What about be32_to_cpu and friends instead of the loops (here and later)?
> 

We can't use be32_to_cpu and friends, because one of the stop condition of this
loop is the length of the msg.

For reading from msg->buf we could consider to mask the irrelevant part.But for
writing to msg->buf it would imply to write outside of the allocated buffer!

Do you agree to not change anything on this patch?

Thanks,


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

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

[Gregory CLEMENT]

On 15/08/2013 14:13, Wolfram Sang wrote:
> 
>> +	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));
> 
> Same comment as in the last version: What about be32_to_cpu and friends instead of the loops (here and later)?
> 

We can't use be32_to_cpu and friends, because one of the stop condition of this
loop is the length of the msg.

For reading from msg->buf we could consider to mask the irrelevant part.But for
writing to msg->buf it would imply to write outside of the allocated buffer!

Do you agree to not change anything on this patch?

Thanks,


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

Re: [PATCH v5 0/3] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

[Gregory CLEMENT]

On 09/08/2013 11:05, Gregory CLEMENT wrote:
> Hello,
> 
> This new version just brings the handle of the "marvell,mv78230-i2"
> string as a single compatible string. As there were no other remarks
> since about 2 months I hope that this version will be able to be
> merged in 3.12.
> 
> 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.
> 
> This series also contains a real fix for the I2C controller of the
> Armada XP SoC.
> 
> The first two patches modify the driver itself and should go through
> i2c subsystem.
> 
> The last patch updates the device tree to be able to use this new
> feature. I kept this patch in the series for having a coherent series,
> but it have already been pulled in the mvebu subsystem.
> 

Wolfram,

Jason Cooper drooped the third patch as it conflicted with a patch from
Maxime Ripard which adds the AllWinner support. Olof also asked a formal
acked-by from a device tree maintainer even if we already answer to
Mark Rutland request.

Olof also requested that you take the binding update, so I am going to send
a new version of this patch set with the last patch split in two parts.

As explained earlier today, unless you really want I use be32_to_cpu in the
mv64xxx_i2c_offload_msg() I won't change anything else.

Regards,

> Thanks,
> 
> Changelog:
> 
> v4-v5:
> 
>  - handle of the "marvell,mv78230-i2" string as a single compatible
>    string. Even it is strongly discouraged to use "marvell,
>    mv78230-i2" alone, it is better to be able to handle it.
> 
> 
> v3->v4:
> 
>  - reverse the order of the compatible strings, with the most
>    specific first
> 
>  - rebased on 3.11-rc1
> 
> v2->v3:
> 
>  - Introduces a new compatible string mv78230-i2c which will be used
>    for the fix and for the offload feature which are only present on
>    the Armada XP SoCs
> 
>  - Removes the unneeded spin_lock_irqsave pointed by Russell King
> 
>  - The offload mechanism is now port of the fsm and handle the
>    multiple messages.
> 
>  - The flag bridge-enabled is renamed to offload_enabled, but the
>    register name stills contains the BRIDGE word to match the
>    datasheet.
> 
>  - Uses writel_relaxed on the place pointed by Russell King
> 
>  - Uses the bool type for the flag (pointed by Thomas Petazzoni)
> 
>  - Removes useless code (pointed by Thomas Petazzoni)
> 
>  - Updates the bindings documentation
> 
> v1->v2:
>  - Move the flag for the timing issue from global scope to per device
>    scope
>  - Assignment is no more done in if condition
> 
> Gregory CLEMENT (3):
>   i2c-mv64xxx: Add I2C Transaction Generator support
>   i2c-mv64xxx: Fix timing issue on Armada XP (errata FE-8471889)
>   ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c
> 
>  .../devicetree/bindings/i2c/i2c-mv64xxx.txt        |  13 +-
>  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                   | 217 ++++++++++++++++++++-
>  5 files changed, 237 insertions(+), 13 deletions(-)
> 


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

[PATCH v5 0/3] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

[Gregory CLEMENT]

On 09/08/2013 11:05, Gregory CLEMENT wrote:
> Hello,
> 
> This new version just brings the handle of the "marvell,mv78230-i2"
> string as a single compatible string. As there were no other remarks
> since about 2 months I hope that this version will be able to be
> merged in 3.12.
> 
> 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.
> 
> This series also contains a real fix for the I2C controller of the
> Armada XP SoC.
> 
> The first two patches modify the driver itself and should go through
> i2c subsystem.
> 
> The last patch updates the device tree to be able to use this new
> feature. I kept this patch in the series for having a coherent series,
> but it have already been pulled in the mvebu subsystem.
> 

Wolfram,

Jason Cooper drooped the third patch as it conflicted with a patch from
Maxime Ripard which adds the AllWinner support. Olof also asked a formal
acked-by from a device tree maintainer even if we already answer to
Mark Rutland request.

Olof also requested that you take the binding update, so I am going to send
a new version of this patch set with the last patch split in two parts.

As explained earlier today, unless you really want I use be32_to_cpu in the
mv64xxx_i2c_offload_msg() I won't change anything else.

Regards,

> Thanks,
> 
> Changelog:
> 
> v4-v5:
> 
>  - handle of the "marvell,mv78230-i2" string as a single compatible
>    string. Even it is strongly discouraged to use "marvell,
>    mv78230-i2" alone, it is better to be able to handle it.
> 
> 
> v3->v4:
> 
>  - reverse the order of the compatible strings, with the most
>    specific first
> 
>  - rebased on 3.11-rc1
> 
> v2->v3:
> 
>  - Introduces a new compatible string mv78230-i2c which will be used
>    for the fix and for the offload feature which are only present on
>    the Armada XP SoCs
> 
>  - Removes the unneeded spin_lock_irqsave pointed by Russell King
> 
>  - The offload mechanism is now port of the fsm and handle the
>    multiple messages.
> 
>  - The flag bridge-enabled is renamed to offload_enabled, but the
>    register name stills contains the BRIDGE word to match the
>    datasheet.
> 
>  - Uses writel_relaxed on the place pointed by Russell King
> 
>  - Uses the bool type for the flag (pointed by Thomas Petazzoni)
> 
>  - Removes useless code (pointed by Thomas Petazzoni)
> 
>  - Updates the bindings documentation
> 
> v1->v2:
>  - Move the flag for the timing issue from global scope to per device
>    scope
>  - Assignment is no more done in if condition
> 
> Gregory CLEMENT (3):
>   i2c-mv64xxx: Add I2C Transaction Generator support
>   i2c-mv64xxx: Fix timing issue on Armada XP (errata FE-8471889)
>   ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c
> 
>  .../devicetree/bindings/i2c/i2c-mv64xxx.txt        |  13 +-
>  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                   | 217 ++++++++++++++++++++-
>  5 files changed, 237 insertions(+), 13 deletions(-)
> 


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

Re: [PATCH v5 0/3] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 722 bytes --]


> Jason Cooper drooped the third patch as it conflicted with a patch from
> Maxime Ripard which adds the AllWinner support. Olof also asked a formal
> acked-by from a device tree maintainer even if we already answer to
> Mark Rutland request.
> 
> Olof also requested that you take the binding update, so I am going to send
> a new version of this patch set with the last patch split in two parts.

Yeah, I wondered about the binding already being accepted when the
driver review was not done.

> As explained earlier today, unless you really want I use be32_to_cpu in the
> mv64xxx_i2c_offload_msg() I won't change anything else.

I found some more (minor) issues. Expect a review by tomorrow evening.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v5 0/3] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

[Wolfram Sang]

> Jason Cooper drooped the third patch as it conflicted with a patch from
> Maxime Ripard which adds the AllWinner support. Olof also asked a formal
> acked-by from a device tree maintainer even if we already answer to
> Mark Rutland request.
> 
> Olof also requested that you take the binding update, so I am going to send
> a new version of this patch set with the last patch split in two parts.

Yeah, I wondered about the binding already being accepted when the
driver review was not done.

> As explained earlier today, unless you really want I use be32_to_cpu in the
> mv64xxx_i2c_offload_msg() I won't change anything else.

I found some more (minor) issues. Expect a review by tomorrow evening.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130820/a803dbd7/attachment.sig>

Re: [PATCH v5 0/3] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

[Gregory CLEMENT]

On 20/08/2013 21:07, Wolfram Sang wrote:
> 
>> Jason Cooper drooped the third patch as it conflicted with a patch from Maxime Ripard which adds the AllWinner support. Olof also asked a formal acked-by from a device tree maintainer
>> even if we already answer to Mark Rutland request.
>> 
>> Olof also requested that you take the binding update, so I am going to send a new version of this patch set with the last patch split in two parts.
> 
> Yeah, I wondered about the binding already being accepted when the driver review was not done.

As there will be a conflict for the
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt file with the
commit "i2c: mv64xxx: Document the newly introduced allwinner
compatible", do you want that I rebase my patch set on your
i2c/for-current branch, or do you prefer to take care of the merge
conflict yourself?

> 
>> As explained earlier today, unless you really want I use be32_to_cpu in the mv64xxx_i2c_offload_msg() I won't change anything else.
> 
> I found some more (minor) issues. Expect a review by tomorrow evening.
> 

Ok I will wait for your review to send the new version

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

[PATCH v5 0/3] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

[Gregory CLEMENT]

On 20/08/2013 21:07, Wolfram Sang wrote:
> 
>> Jason Cooper drooped the third patch as it conflicted with a patch from Maxime Ripard which adds the AllWinner support. Olof also asked a formal acked-by from a device tree maintainer
>> even if we already answer to Mark Rutland request.
>> 
>> Olof also requested that you take the binding update, so I am going to send a new version of this patch set with the last patch split in two parts.
> 
> Yeah, I wondered about the binding already being accepted when the driver review was not done.

As there will be a conflict for the
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt file with the
commit "i2c: mv64xxx: Document the newly introduced allwinner
compatible", do you want that I rebase my patch set on your
i2c/for-current branch, or do you prefer to take care of the merge
conflict yourself?

> 
>> As explained earlier today, unless you really want I use be32_to_cpu in the mv64xxx_i2c_offload_msg() I won't change anything else.
> 
> I found some more (minor) issues. Expect a review by tomorrow evening.
> 

Ok I will wait for your review to send the new version

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

Re: [PATCH v5 0/3] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 187 bytes --]


> compatible", do you want that I rebase my patch set on your
> i2c/for-current branch, or do you prefer to take care of the merge
> conflict yourself?

My for-next would be preferred.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v5 0/3] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

[Wolfram Sang]

> compatible", do you want that I rebase my patch set on your
> i2c/for-current branch, or do you prefer to take care of the merge
> conflict yourself?

My for-next would be preferred.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130821/2b791dc9/attachment.sig>

Re: [PATCH v5 1/3] i2c-mv64xxx: Add I2C Transaction Generator support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 4757 bytes --]

Hi,

here is the review. BTW have you tested with and without the offload
engine?

> +static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
> +{
> +	unsigned long data_reg_hi = 0;
> +	unsigned long data_reg_lo = 0;
> +	unsigned long ctrl_reg;
> +	unsigned int i;
> +	struct i2c_msg *msg = drv_data->msgs;
> +
> +	drv_data->msg = msg;
> +	drv_data->byte_posn = 0;
> +	drv_data->bytes_left = msg->len;
> +	drv_data->aborting = 0;
> +	drv_data->rc = 0;
> +	/* Only regular transactions can be offloaded */
> +	if ((msg->flags & ~(I2C_M_TEN | I2C_M_RD)) != 0)
> +		return 1;

-EINVAL?

> +
> +	/* Only 1-8 byte transfers can be offloaded */
> +	if (msg->len < 1 || msg->len > 8)
> +		return 1;

ditto

> +
> +	/* 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));

What about:

	local_buf[8] = { 0 };

	memcpy(local_buf, msg->buf, msg->len);
	cpu_to_be32(...)

? A lot less lines and be32 macros are likely more efficient. Copy loop
probably, too.

> +
> +		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 */
> +	writel_relaxed(data_reg_lo,
> +		drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_LO);
> +	writel_relaxed(data_reg_hi,
> +		drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_HI);

Do you need to write the 0 in case of I2C_M_RD?

> +	writel(ctrl_reg, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
> +
> +	return 0;
> +}
> +
> +static void
> +mv64xxx_i2c_update_offload_data(struct i2c_msg *msg, unsigned long data_reg_hi,
> +	unsigned long data_reg_lo)
> +{
> +	int i;
> +
> +	if ((msg->flags & I2C_M_RD) != 0) {

!= 0 is superfluous

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

Same idea as above?

> @@ -298,21 +420,36 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
>  static void
>  mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  {
> +	unsigned long data_reg_hi = 0;
> +	unsigned long data_reg_lo = 0;
> +
>  	switch(drv_data->action) {
> +	case MV64XXX_I2C_ACTION_OFFLOAD_RESTART:
> +		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);

Initializing data_reg_* is the same for both calls to
update_offload_data, so it could be moved into the function.
Probably not needed when using the local_buf idea.

> @@ -326,6 +463,12 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  			drv_data->reg_base + drv_data->reg_offsets.control);
>  		break;
>  
> +	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
> +		if (mv64xxx_i2c_offload_msg(drv_data) <= 0)

needs to be adjusted when using -EINVAL above. I'd prefer the error case
in the else branch, though. Easier to read.

> +			break;
> +		else
> +			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
> +		/* FALLTHRU */
>  	case MV64XXX_I2C_ACTION_SEND_START:
>  		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
>  			drv_data->reg_base + drv_data->reg_offsets.control);

> @@ -601,6 +779,13 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
>  
>  	memcpy(&drv_data->reg_offsets, device->data, sizeof(drv_data->reg_offsets));
>  
> +	/*
> +	 * For controllers embedded in new SoCs activate the
> +	 * Transaction Generator support.
> +	 */
> +	if (of_device_is_compatible(np, "marvell,mv78230-i2c"))
> +		drv_data->offload_enabled = true;

For now OK, if there are more users, someone will need to convert it to
be included in match_data.

> @@ -654,6 +839,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->offload_enabled = 0;

'false' instead of 0.

Regards,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Wolfram Sang]

Hi,

here is the review. BTW have you tested with and without the offload
engine?

> +static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
> +{
> +	unsigned long data_reg_hi = 0;
> +	unsigned long data_reg_lo = 0;
> +	unsigned long ctrl_reg;
> +	unsigned int i;
> +	struct i2c_msg *msg = drv_data->msgs;
> +
> +	drv_data->msg = msg;
> +	drv_data->byte_posn = 0;
> +	drv_data->bytes_left = msg->len;
> +	drv_data->aborting = 0;
> +	drv_data->rc = 0;
> +	/* Only regular transactions can be offloaded */
> +	if ((msg->flags & ~(I2C_M_TEN | I2C_M_RD)) != 0)
> +		return 1;

-EINVAL?

> +
> +	/* Only 1-8 byte transfers can be offloaded */
> +	if (msg->len < 1 || msg->len > 8)
> +		return 1;

ditto

> +
> +	/* 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));

What about:

	local_buf[8] = { 0 };

	memcpy(local_buf, msg->buf, msg->len);
	cpu_to_be32(...)

? A lot less lines and be32 macros are likely more efficient. Copy loop
probably, too.

> +
> +		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 */
> +	writel_relaxed(data_reg_lo,
> +		drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_LO);
> +	writel_relaxed(data_reg_hi,
> +		drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_HI);

Do you need to write the 0 in case of I2C_M_RD?

> +	writel(ctrl_reg, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
> +
> +	return 0;
> +}
> +
> +static void
> +mv64xxx_i2c_update_offload_data(struct i2c_msg *msg, unsigned long data_reg_hi,
> +	unsigned long data_reg_lo)
> +{
> +	int i;
> +
> +	if ((msg->flags & I2C_M_RD) != 0) {

!= 0 is superfluous

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

Same idea as above?

> @@ -298,21 +420,36 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
>  static void
>  mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  {
> +	unsigned long data_reg_hi = 0;
> +	unsigned long data_reg_lo = 0;
> +
>  	switch(drv_data->action) {
> +	case MV64XXX_I2C_ACTION_OFFLOAD_RESTART:
> +		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);

Initializing data_reg_* is the same for both calls to
update_offload_data, so it could be moved into the function.
Probably not needed when using the local_buf idea.

> @@ -326,6 +463,12 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  			drv_data->reg_base + drv_data->reg_offsets.control);
>  		break;
>  
> +	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
> +		if (mv64xxx_i2c_offload_msg(drv_data) <= 0)

needs to be adjusted when using -EINVAL above. I'd prefer the error case
in the else branch, though. Easier to read.

> +			break;
> +		else
> +			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
> +		/* FALLTHRU */
>  	case MV64XXX_I2C_ACTION_SEND_START:
>  		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
>  			drv_data->reg_base + drv_data->reg_offsets.control);

> @@ -601,6 +779,13 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
>  
>  	memcpy(&drv_data->reg_offsets, device->data, sizeof(drv_data->reg_offsets));
>  
> +	/*
> +	 * For controllers embedded in new SoCs activate the
> +	 * Transaction Generator support.
> +	 */
> +	if (of_device_is_compatible(np, "marvell,mv78230-i2c"))
> +		drv_data->offload_enabled = true;

For now OK, if there are more users, someone will need to convert it to
be included in match_data.

> @@ -654,6 +839,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->offload_enabled = 0;

'false' instead of 0.

Regards,

   Wolfram

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130821/69fc2173/attachment-0001.sig>

Re: [PATCH v5 1/3] i2c-mv64xxx: Add I2C Transaction Generator support

[Gregory CLEMENT]

On 21/08/2013 23:01, Wolfram Sang wrote:
> Hi,
> 
> here is the review. BTW have you tested with and without the offload
> engine?

yes with eeprog.

> 
>> +static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
>> +{
>> +	unsigned long data_reg_hi = 0;
>> +	unsigned long data_reg_lo = 0;
>> +	unsigned long ctrl_reg;
>> +	unsigned int i;
>> +	struct i2c_msg *msg = drv_data->msgs;
>> +
>> +	drv_data->msg = msg;
>> +	drv_data->byte_posn = 0;
>> +	drv_data->bytes_left = msg->len;
>> +	drv_data->aborting = 0;
>> +	drv_data->rc = 0;
>> +	/* Only regular transactions can be offloaded */
>> +	if ((msg->flags & ~(I2C_M_TEN | I2C_M_RD)) != 0)
>> +		return 1;
> 
> -EINVAL?
> 

OK

>> +
>> +	/* Only 1-8 byte transfers can be offloaded */
>> +	if (msg->len < 1 || msg->len > 8)
>> +		return 1;
> 
> ditto
> 

OK

>> +
>> +	/* 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));
> 
> What about:
> 
> 	local_buf[8] = { 0 };
> 
> 	memcpy(local_buf, msg->buf, msg->len);
> 	cpu_to_be32(...)
> 
> ? A lot less lines and be32 macros are likely more efficient. Copy loop
> probably, too.
> 

OK


>> +
>> +		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 */
>> +	writel_relaxed(data_reg_lo,
>> +		drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_LO);
>> +	writel_relaxed(data_reg_hi,
>> +		drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_HI);
> 
> Do you need to write the 0 in case of I2C_M_RD?
> 

Not sure I will check it

>> +	writel(ctrl_reg, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
>> +
>> +	return 0;
>> +}
>> +
>> +static void
>> +mv64xxx_i2c_update_offload_data(struct i2c_msg *msg, unsigned long data_reg_hi,
>> +	unsigned long data_reg_lo)
>> +{
>> +	int i;
>> +
>> +	if ((msg->flags & I2C_M_RD) != 0) {
> 
> != 0 is superfluous
> 

OK

>> +		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;
>> +		}
>> +	}
> 
> Same idea as above?
> 

OK I will do it here also as in both cases the number of lines will be
shorter, but for small amount of data I am not sure it will be faster.

>> @@ -298,21 +420,36 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
>>  static void
>>  mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>>  {
>> +	unsigned long data_reg_hi = 0;
>> +	unsigned long data_reg_lo = 0;
>> +
>>  	switch(drv_data->action) {
>> +	case MV64XXX_I2C_ACTION_OFFLOAD_RESTART:
>> +		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);
> 
> Initializing data_reg_* is the same for both calls to
> update_offload_data, so it could be moved into the function.
> Probably not needed when using the local_buf idea.
> 

I will move this part in the update_offload_data function.

>> @@ -326,6 +463,12 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>>  			drv_data->reg_base + drv_data->reg_offsets.control);
>>  		break;
>>  
>> +	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
>> +		if (mv64xxx_i2c_offload_msg(drv_data) <= 0)
> 
> needs to be adjusted when using -EINVAL above. I'd prefer the error case
> in the else branch, though. Easier to read.
> 

OK, but in this case ...

>> +			break;
>> +		else
>> +			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
>> +		/* FALLTHRU */

... the fall through here is less readable. But it is a matter of
taste, I will change this.

>>  	case MV64XXX_I2C_ACTION_SEND_START:
>>  		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
>>  			drv_data->reg_base + drv_data->reg_offsets.control);
> 
>> @@ -601,6 +779,13 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
>>  
>>  	memcpy(&drv_data->reg_offsets, device->data, sizeof(drv_data->reg_offsets));
>>  
>> +	/*
>> +	 * For controllers embedded in new SoCs activate the
>> +	 * Transaction Generator support.
>> +	 */
>> +	if (of_device_is_compatible(np, "marvell,mv78230-i2c"))
>> +		drv_data->offload_enabled = true;
> 
> For now OK, if there are more users, someone will need to convert it to
> be included in match_data.
> 
>> @@ -654,6 +839,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->offload_enabled = 0;
> 
> 'false' instead of 0.
> 

OK

Thanks for your review.

Gregory

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

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

[Gregory CLEMENT]

On 21/08/2013 23:01, Wolfram Sang wrote:
> Hi,
> 
> here is the review. BTW have you tested with and without the offload
> engine?

yes with eeprog.

> 
>> +static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
>> +{
>> +	unsigned long data_reg_hi = 0;
>> +	unsigned long data_reg_lo = 0;
>> +	unsigned long ctrl_reg;
>> +	unsigned int i;
>> +	struct i2c_msg *msg = drv_data->msgs;
>> +
>> +	drv_data->msg = msg;
>> +	drv_data->byte_posn = 0;
>> +	drv_data->bytes_left = msg->len;
>> +	drv_data->aborting = 0;
>> +	drv_data->rc = 0;
>> +	/* Only regular transactions can be offloaded */
>> +	if ((msg->flags & ~(I2C_M_TEN | I2C_M_RD)) != 0)
>> +		return 1;
> 
> -EINVAL?
> 

OK

>> +
>> +	/* Only 1-8 byte transfers can be offloaded */
>> +	if (msg->len < 1 || msg->len > 8)
>> +		return 1;
> 
> ditto
> 

OK

>> +
>> +	/* 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));
> 
> What about:
> 
> 	local_buf[8] = { 0 };
> 
> 	memcpy(local_buf, msg->buf, msg->len);
> 	cpu_to_be32(...)
> 
> ? A lot less lines and be32 macros are likely more efficient. Copy loop
> probably, too.
> 

OK


>> +
>> +		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 */
>> +	writel_relaxed(data_reg_lo,
>> +		drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_LO);
>> +	writel_relaxed(data_reg_hi,
>> +		drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_HI);
> 
> Do you need to write the 0 in case of I2C_M_RD?
> 

Not sure I will check it

>> +	writel(ctrl_reg, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
>> +
>> +	return 0;
>> +}
>> +
>> +static void
>> +mv64xxx_i2c_update_offload_data(struct i2c_msg *msg, unsigned long data_reg_hi,
>> +	unsigned long data_reg_lo)
>> +{
>> +	int i;
>> +
>> +	if ((msg->flags & I2C_M_RD) != 0) {
> 
> != 0 is superfluous
> 

OK

>> +		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;
>> +		}
>> +	}
> 
> Same idea as above?
> 

OK I will do it here also as in both cases the number of lines will be
shorter, but for small amount of data I am not sure it will be faster.

>> @@ -298,21 +420,36 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
>>  static void
>>  mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>>  {
>> +	unsigned long data_reg_hi = 0;
>> +	unsigned long data_reg_lo = 0;
>> +
>>  	switch(drv_data->action) {
>> +	case MV64XXX_I2C_ACTION_OFFLOAD_RESTART:
>> +		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);
> 
> Initializing data_reg_* is the same for both calls to
> update_offload_data, so it could be moved into the function.
> Probably not needed when using the local_buf idea.
> 

I will move this part in the update_offload_data function.

>> @@ -326,6 +463,12 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>>  			drv_data->reg_base + drv_data->reg_offsets.control);
>>  		break;
>>  
>> +	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
>> +		if (mv64xxx_i2c_offload_msg(drv_data) <= 0)
> 
> needs to be adjusted when using -EINVAL above. I'd prefer the error case
> in the else branch, though. Easier to read.
> 

OK, but in this case ...

>> +			break;
>> +		else
>> +			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
>> +		/* FALLTHRU */

... the fall through here is less readable. But it is a matter of
taste, I will change this.

>>  	case MV64XXX_I2C_ACTION_SEND_START:
>>  		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
>>  			drv_data->reg_base + drv_data->reg_offsets.control);
> 
>> @@ -601,6 +779,13 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
>>  
>>  	memcpy(&drv_data->reg_offsets, device->data, sizeof(drv_data->reg_offsets));
>>  
>> +	/*
>> +	 * For controllers embedded in new SoCs activate the
>> +	 * Transaction Generator support.
>> +	 */
>> +	if (of_device_is_compatible(np, "marvell,mv78230-i2c"))
>> +		drv_data->offload_enabled = true;
> 
> For now OK, if there are more users, someone will need to convert it to
> be included in match_data.
> 
>> @@ -654,6 +839,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->offload_enabled = 0;
> 
> 'false' instead of 0.
> 

OK

Thanks for your review.

Gregory

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

Re: [PATCH v5 1/3] i2c-mv64xxx: Add I2C Transaction Generator support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 725 bytes --]


> >> @@ -326,6 +463,12 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
> >>  			drv_data->reg_base + drv_data->reg_offsets.control);
> >>  		break;
> >>  
> >> +	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
> >> +		if (mv64xxx_i2c_offload_msg(drv_data) <= 0)
> > 
> > needs to be adjusted when using -EINVAL above. I'd prefer the error case
> > in the else branch, though. Easier to read.
> > 
> 
> OK, but in this case ...
> 
> >> +			break;
> >> +		else
> >> +			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
> >> +		/* FALLTHRU */
> 
> ... the fall through here is less readable. But it is a matter of
> taste, I will change this.

Ah, I see. Well, try both and decide.

Thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Wolfram Sang]

> >> @@ -326,6 +463,12 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
> >>  			drv_data->reg_base + drv_data->reg_offsets.control);
> >>  		break;
> >>  
> >> +	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
> >> +		if (mv64xxx_i2c_offload_msg(drv_data) <= 0)
> > 
> > needs to be adjusted when using -EINVAL above. I'd prefer the error case
> > in the else branch, though. Easier to read.
> > 
> 
> OK, but in this case ...
> 
> >> +			break;
> >> +		else
> >> +			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
> >> +		/* FALLTHRU */
> 
> ... the fall through here is less readable. But it is a matter of
> taste, I will change this.

Ah, I see. Well, try both and decide.

Thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130822/3a92435b/attachment-0001.sig>