i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

PATCH Superseded Gregory CLEMENT 4 participants 17 messages Aug 9, 2013 – Aug 22, 2013

Subsystems (3)

Device tree bindings I2C subsystem host drivers I2C MV64XXX Marvell and Allwinner driver

lore

Share & tools

lore

Gregory CLEMENT

author

2013-08-09 09:05 UTC #0 lore raw

v5 cover i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

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(-)

Signature · 1 line

1.8.1.2

Gregory CLEMENT

author

2013-08-09 09:05 UTC #1 lore raw

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

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 <[email protected]>

Signed-off-by Gregory CLEMENT <[email protected]>

---
 drivers/i2c/busses/i2c-mv64xxx.c | 208 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 197 insertions(+), 11 deletions(-)

drivers/i2c/busses/i2c-mv64xxx.c +197 ~~-11~~

index b1f42bf..406d9c6 100644
@@ -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);

Signature · 1 line

1.8.1.2

Wolfram Sang

2013-08-15 12:13 UTC #2 lore raw

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

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, · line 233

Quoted 9 lines

drivers/i2c/busses/i2c-mv64xxx.c +8 -0

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

Gregory CLEMENT

author

2013-08-20 15:56 UTC #3 lore raw

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

Quoted 6 lines · Wolfram Sang

Quoted 3 lines
(quoted diff) +1 -0
+	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,

Signature · 4 lines

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

Wolfram Sang

2013-08-21 21:01 UTC #4 lore raw

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

In reply to Gregory CLEMENT

Hi,

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

Quoted 6 lines

drivers/i2c/busses/i2c-mv64xxx.c +6 -0

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

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, · line 213

Quoted 10 lines

drivers/i2c/busses/i2c-mv64xxx.c +10 -0

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

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, · line 217

Quoted 4 lines

drivers/i2c/busses/i2c-mv64xxx.c +4 -0

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

ditto

Quoted 6 lines

drivers/i2c/busses/i2c-mv64xxx.c +6 -0

+
+	/* Build transaction */
+	ctrl_reg = MV64XXX_I2C_BRIDGE_CONTROL_ENABLE |
+		   (msg->addr << MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT);
+
+	if ((msg->flags & I2C_M_TEN) != 0)

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, · line 233

Quoted 10 lines

drivers/i2c/busses/i2c-mv64xxx.c +10 -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.

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, · line 246

Quoted 13 lines

drivers/i2c/busses/i2c-mv64xxx.c +13 -0

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

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, · line 258

Quoted 12 lines

drivers/i2c/busses/i2c-mv64xxx.c +12 -0

+	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

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, · line 268

Quoted 10 lines

drivers/i2c/busses/i2c-mv64xxx.c +10 -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;
+		}
+	}

Same idea as above?

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status) · line 431

Quoted 13 lines

drivers/i2c/busses/i2c-mv64xxx.c +8 -0

@@ -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.

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) · line 467

Quoted 6 lines

drivers/i2c/busses/i2c-mv64xxx.c +2 -0

@@ -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.

Quoted 9 lines

drivers/i2c/busses/i2c-mv64xxx.c +4 -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);

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, · line 787

Quoted 9 lines

drivers/i2c/busses/i2c-mv64xxx.c +6 -0

@@ -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.

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_probe(struct platform_device *pd) · line 842

Quoted 5 lines

drivers/i2c/busses/i2c-mv64xxx.c +1 -0

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

Gregory CLEMENT

author

2013-08-22 07:40 UTC #5 lore raw

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

Quoted 4 lines · Wolfram Sang

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

yes with eeprog.

Quoted 11 lines

drivers/i2c/busses/i2c-mv64xxx.c +10 -0

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

Quoted 9 lines

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, · line 213
Quoted 6 lines
drivers/i2c/busses/i2c-mv64xxx.c +6 -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?

Quoted 7 lines

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, · line 217
Quoted 4 lines
drivers/i2c/busses/i2c-mv64xxx.c +4 -0
+
+	/* Only 1-8 byte transfers can be offloaded */
+	if (msg->len < 1 || msg->len > 8)
+		return 1;
ditto

Quoted 19 lines

Quoted 6 lines

drivers/i2c/busses/i2c-mv64xxx.c +6 -0

+
+	/* Build transaction */
+	ctrl_reg = MV64XXX_I2C_BRIDGE_CONTROL_ENABLE |
+		   (msg->addr << MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT);
+
+	if ((msg->flags & I2C_M_TEN) != 0)

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, · line 233

Quoted 10 lines

drivers/i2c/busses/i2c-mv64xxx.c +10 -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:

Quoted 8 lines

	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.

Quoted 7 lines

drivers/i2c/busses/i2c-mv64xxx.c +7 -0

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

Quoted 9 lines

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, · line 246
Quoted 6 lines
drivers/i2c/busses/i2c-mv64xxx.c +6 -0
+
+	/* 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

Quoted 6 lines

drivers/i2c/busses/i2c-mv64xxx.c +6 -0

+	writel(ctrl_reg, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
+
+	return 0;
+}
+
+static void

Quoted 9 lines

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, · line 258
Quoted 6 lines
drivers/i2c/busses/i2c-mv64xxx.c +6 -0
+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

Quoted 4 lines

drivers/i2c/busses/i2c-mv64xxx.c +4 -0

+		for (i = 0; i < 4 && i < msg->len; i++) {
+			msg->buf[i] = data_reg_lo & 0xFF;
+			data_reg_lo >>= 8;
+		}

Quoted 9 lines

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, · line 268
Quoted 6 lines
drivers/i2c/busses/i2c-mv64xxx.c +6 -0
+
+		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.

Quoted 9 lines

drivers/i2c/busses/i2c-mv64xxx.c +4 -0

@@ -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:

Quoted 9 lines

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status) · line 431
Quoted 4 lines
drivers/i2c/busses/i2c-mv64xxx.c +4 -0
+		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.

Quoted 10 lines

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) · line 467
Quoted 6 lines
drivers/i2c/busses/i2c-mv64xxx.c +2 -0
@@ -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 ...

Quoted 4 lines

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) · line 471
Quoted 4 lines
drivers/i2c/busses/i2c-mv64xxx.c +4 -0
+			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.

Quoted 18 lines

Quoted 5 lines
drivers/i2c/busses/i2c-mv64xxx.c +0 -0
 	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);
drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, · line 787
Quoted 9 lines
drivers/i2c/busses/i2c-mv64xxx.c +6 -0
@@ -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.

Quoted 8 lines

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_probe(struct platform_device *pd) · line 842
Quoted 5 lines
drivers/i2c/busses/i2c-mv64xxx.c +1 -0
@@ -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.

Thanks for your review.

Gregory

Signature · 4 lines

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

Wolfram Sang

2013-08-22 09:06 UTC #6 lore raw

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

Quoted 14 lines

Quoted 11 lines
drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) · line 467
Quoted 7 lines
drivers/i2c/busses/i2c-mv64xxx.c +2 -0
@@ -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 ...

Quoted 7 lines

Quoted 4 lines
drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) · line 471
Quoted 4 lines
drivers/i2c/busses/i2c-mv64xxx.c +4 -0
+			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!

Gregory CLEMENT

author

2013-08-09 09:05 UTC #7 lore raw

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

In reply to 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 <[email protected]>

Signed-off-by Zbigniew Bodek <[email protected]>

---
 drivers/i2c/busses/i2c-mv64xxx.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-mv64xxx.c +13 -2

index 406d9c6..a78d7ee 100644
@@ -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;

Signature · 1 line

1.8.1.2

Gregory CLEMENT

author

2013-08-09 09:05 UTC #8 lore raw

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

In reply to 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 <[email protected]>

---
 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(-)

Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +12 -1

index a1ee681..c5dd952 100644
@@ -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>;
+	};

arch/arm/boot/dts/armada-370-xp.dtsi +0 -2

index 90b1176..d8b24c9 100644
@@ -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>;

arch/arm/boot/dts/armada-370.dtsi +8 -0

index fa3dfc6..0e2eefa 100644
@@ -155,6 +155,14 @@
 				};
 			};
 
+			i2c0: i2c at 11000 {
+				reg = <0x11000 0x20>;
+			};
+
+			i2c1: i2c at 11100 {
+				reg = <0x11100 0x20>;
+			};
+
 			usb at 50000 {
 				clocks = <&coreclk 0>;
 			};

arch/arm/boot/dts/armada-xp.dtsi +10 -0

index 416eb94..e1f2547 100644
@@ -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>;
 			};

Signature · 1 line

1.8.1.2

Ezequiel Garcia

2013-08-09 09:13 UTC #9 lore raw

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

(Sending to devicetree)

Quoted 112 lines · Gregory CLEMENT

The Device Tree binding documentation is updated accordingly.

Signed-off-by Gregory CLEMENT <[email protected]>

---
 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(-)

Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +12 -1

index a1ee681..c5dd952 100644
@@ -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>;
+	};

arch/arm/boot/dts/armada-370-xp.dtsi +0 -2

index 90b1176..d8b24c9 100644
@@ -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>;

arch/arm/boot/dts/armada-370.dtsi +8 -0

index fa3dfc6..0e2eefa 100644
@@ -155,6 +155,14 @@
 				};
 			};
 
+			i2c0: i2c at 11000 {
+				reg = <0x11000 0x20>;
+			};
+
+			i2c1: i2c at 11100 {
+				reg = <0x11100 0x20>;
+			};
+
 			usb at 50000 {
 				clocks = <&coreclk 0>;
 			};

arch/arm/boot/dts/armada-xp.dtsi +10 -0

index 416eb94..e1f2547 100644
@@ -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>;
 			};

Signature · 1 line

1.8.1.2

Signature · 3 lines

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

Gregory CLEMENT

author

2013-08-09 09:18 UTC #10 lore raw

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

Quoted 1 line · Ezequiel Garcia

(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.

Quoted 115 lines · Gregory CLEMENT

Quoted 112 lines

The Device Tree binding documentation is updated accordingly.

Signed-off-by Gregory CLEMENT <[email protected]>

---
 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(-)

Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +12 -1

index a1ee681..c5dd952 100644
@@ -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>;
+	};

arch/arm/boot/dts/armada-370-xp.dtsi +0 -2

index 90b1176..d8b24c9 100644
@@ -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>;

arch/arm/boot/dts/armada-370.dtsi +8 -0

index fa3dfc6..0e2eefa 100644
@@ -155,6 +155,14 @@
 				};
 			};
 
+			i2c0: i2c at 11000 {
+				reg = <0x11000 0x20>;
+			};
+
+			i2c1: i2c at 11100 {
+				reg = <0x11100 0x20>;
+			};
+
 			usb at 50000 {
 				clocks = <&coreclk 0>;
 			};

arch/arm/boot/dts/armada-xp.dtsi +10 -0

index 416eb94..e1f2547 100644
@@ -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>;
 			};

Signature · 1 line

1.8.1.2

Signature · 4 lines

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

Ezequiel Garcia

2013-08-09 09:30 UTC #11 lore raw

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

Quoted 7 lines · Gregory CLEMENT

Quoted 1 line · Ezequiel Garcia
(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.

Signature · 3 lines

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

Jason Cooper

2013-08-09 11:32 UTC #12 lore raw

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

In reply to Ezequiel Garcia

Ezequiel,

Quoted 1 line · Ezequiel Garcia

(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.

Quoted 122 lines · Gregory CLEMENT

Quoted 112 lines

The Device Tree binding documentation is updated accordingly.

Signed-off-by Gregory CLEMENT <[email protected]>

---
 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(-)

Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +12 -1

index a1ee681..c5dd952 100644
@@ -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>;
+	};

arch/arm/boot/dts/armada-370-xp.dtsi +0 -2

index 90b1176..d8b24c9 100644
@@ -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>;

arch/arm/boot/dts/armada-370.dtsi +8 -0

index fa3dfc6..0e2eefa 100644
@@ -155,6 +155,14 @@
 				};
 			};
 
+			i2c0: i2c at 11000 {
+				reg = <0x11000 0x20>;
+			};
+
+			i2c1: i2c at 11100 {
+				reg = <0x11100 0x20>;
+			};
+
 			usb at 50000 {
 				clocks = <&coreclk 0>;
 			};

arch/arm/boot/dts/armada-xp.dtsi +10 -0

index 416eb94..e1f2547 100644
@@ -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>;
 			};

Signature · 1 line

1.8.1.2

Signature · 7 lines

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

Gregory CLEMENT

author

2013-08-20 16:21 UTC #13 lore raw

v5 cover i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

In reply to Gregory CLEMENT

Quoted 15 lines · 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.

Quoted 7 lines

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,

Quoted 58 lines

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(-)

Signature · 4 lines

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

Wolfram Sang

2013-08-20 19:07 UTC #14 lore raw

v5 cover i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

Quoted 8 lines

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.

Quoted 2 lines

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.

Gregory CLEMENT

author

2013-08-21 10:44 UTC #15 lore raw

v5 cover i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

Quoted 7 lines · Wolfram Sang

Quoted 5 lines
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?

Quoted 5 lines

Quoted 2 lines
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,

Signature · 4 lines

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

Wolfram Sang

2013-08-21 13:43 UTC #16 lore raw

v5 cover i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

Quoted 4 lines

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.