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

PATCH Superseded Gregory CLEMENT 5 participants 17 messages Jun 21, 2013 – Jun 26, 2013

Subsystems (3)

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

lore

Share & tools

lore

Gregory CLEMENT

author

2013-06-21 13:32 UTC #0 lore raw

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

Hello,

this patch set adds support for the I2C Transaction Generator which offloads CPU from managing I2C transfer step by step. This feature is currently only available on the I2C controller IP embedded in the Armada XP SoC.

This series also contains a real fix for the I2C controller of the Armada XP SoC and a patch closer to an improvement than a fix.

The first three 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 and should go through mvebu subsystem.

Due to the recent changes in i2c-mv64xxx.c file, I based these patches on the for-next branch of the i2c git repository.

Thanks,

Change log:

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 (4):
i2c-mv64xxx: Set bus frequency to 100kHz if clock-frequency is not
provided
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        |  19 +-
 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                   | 222 ++++++++++++++++++++-
 5 files changed, 246 insertions(+), 15 deletions(-)

Signature · 1 line

1.8.1.2

Gregory CLEMENT

author

2013-06-21 13:32 UTC #1 lore raw

PATCH v3 1/4 i2c-mv64xxx: Set bus frequency to 100kHz if clock-frequency is not provided

This commit adds checking whether clock-frequency property acquisition has succeeded. If not, the frequency is set to 100kHz by default.

The Device Tree binding documentation is updated accordingly.

Based on the intials patches from Zbigniew Bodek

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

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

---
 Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 6 +++++-
 drivers/i2c/busses/i2c-mv64xxx.c                      | 6 +++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

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

index f46d928..a1ee681 100644
@@ -6,7 +6,11 @@ Required properties :
  - reg             : Offset and length of the register set for the device
  - compatible      : Should be "marvell,mv64xxx-i2c"
  - interrupts      : The interrupt number
- - clock-frequency : Desired I2C bus clock frequency in Hz.
+
+Optional properties :
+
+ - clock-frequency : Desired I2C bus clock frequency in Hz. If not set the
+default frequency is 100kHz
 
 Examples:

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

index 7a0e39b..d5d46db 100644
@@ -578,7 +578,11 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 		goto out;
 	}
 	tclk = clk_get_rate(drv_data->clk);
-	of_property_read_u32(np, "clock-frequency", &bus_freq);
+
+	rc = of_property_read_u32(np, "clock-frequency", &bus_freq);
+	if (rc)
+		bus_freq = 100000; /* 100kHz by default */
+
 	if (!mv64xxx_find_baud_factors(bus_freq, tclk,
 				       &drv_data->freq_n, &drv_data->freq_m)) {
 		rc = -EINVAL;

Signature · 1 line

1.8.1.2

Wolfram Sang

Applied

2013-06-25 21:44 UTC #2 lore raw

PATCH v3 1/4 i2c-mv64xxx: Set bus frequency to 100kHz if clock-frequency is not provided

Quoted 9 lines · Gregory CLEMENT

This commit adds checking whether clock-frequency property acquisition has succeeded. If not, the frequency is set to 100kHz by default.
The Device Tree binding documentation is updated accordingly.
Based on the intials patches from Zbigniew Bodek
Signed-off-by Gregory CLEMENT <[email protected]>
Signed-off-by Zbigniew Bodek <[email protected]>

Applied to for-next, thanks! Rest of the series is 3.12 material.

-------------- 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/20130625/707c68b3/attachment.sig>

Gregory CLEMENT

author

2013-06-26 07:55 UTC #3 lore raw

PATCH v3 1/4 i2c-mv64xxx: Set bus frequency to 100kHz if clock-frequency is not provided

Quoted 11 lines · Wolfram Sang

Quoted 7 lines · Gregory CLEMENT
This commit adds checking whether clock-frequency property acquisition has succeeded. If not, the frequency is set to 100kHz by default.
The Device Tree binding documentation is updated accordingly.
Based on the intials patches from Zbigniew Bodek
Signed-off-by Gregory CLEMENT <[email protected]> Signed-off-by: Zbigniew Bodek <[email protected]>
Applied to for-next, thanks! Rest of the series is 3.12 material.

Thanks!

For the rest of the series, does it mean that you agree with it, or that you didn't review it yet?

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-06-26 13:41 UTC #4 lore raw

PATCH v3 1/4 i2c-mv64xxx: Set bus frequency to 100kHz if clock-frequency is not provided

Quoted 3 lines

For the rest of the series, does it mean that you agree with it, or that you didn't review it yet?

Didn't review. So, other people can still join in ;)

Gregory CLEMENT

author

2013-06-21 13:32 UTC #5 lore raw

PATCH v3 2/4 i2c-mv64xxx: Add I2C Transaction Generator support

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

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

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

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

index d5d46db..c07e742 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 = -ENODEV;
 		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);

@@ -601,6 +778,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 +838,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

Gregory CLEMENT

author

2013-06-21 13:32 UTC #6 lore raw

PATCH v3 3/4 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 c07e742..e5b9c42 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;

@@ -780,10 +789,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-06-21 13:32 UTC #7 lore raw

PATCH v3 4/4 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..ce7af6a 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,mv64xxx-i2c", "marvell,mv78230-i2c";
+		reg = <0x11000 0x100>;
+		compatible = "marvell,mv64xxx-i2c";
+		interrupts = <29>;
+		clock-frequency = <100000>;
+	};

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

index 550eb77..b6f475c 100644
@@ -112,7 +112,6 @@
 
 			i2c0: i2c at 11000 {
 				compatible = "marvell,mv64xxx-i2c";
-				reg = <0x11000 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				interrupts = <31>;

@@ -123,7 +122,6 @@
 
 			i2c1: i2c at 11100 {
 				compatible = "marvell,mv64xxx-i2c";
-				reg = <0x11100 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				interrupts = <32>;

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

index aee2b18..39b26d6 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 5b902f9..c2c357e 100644
@@ -134,6 +134,16 @@
 				};
 			};
 
+			i2c0: i2c at 11000 {
+				compatible = "marvell,mv64xxx-i2c", "marvell,mv78230-i2c";
+				reg = <0x11000 0x100>;
+			};
+
+			i2c1: i2c at 11100 {
+				compatible = "marvell,mv64xxx-i2c", "marvell,mv78230-i2c";
+				reg = <0x11100 0x100>;
+			};
+
 			usb at 50000 {
 				clocks = <&gateclk 18>;
 			};

Signature · 1 line

1.8.1.2

Jason Cooper

2013-06-21 14:07 UTC #8 lore raw

PATCH v3 4/4 ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

Quoted 36 lines · 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 +2 -1
index a1ee681..ce7af6a 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 :

Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt Examples: · line 30

Quoted 10 lines

Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +7 -0

@@ -20,3 +21,13 @@ Examples:
 		interrupts = <29>;
 		clock-frequency = <100000>;
 	};
+
+For a controller which support the I2C Transaction Generator:
+
+	i2c at 11000 {
+		compatible = "marvell,mv64xxx-i2c", "marvell,mv78230-i2c";
+		reg = <0x11000 0x100>;
+		compatible = "marvell,mv64xxx-i2c";

extra compatible line. If there's nothing else, I'll fix this up when I pull it in.

thx,

Jason.

Quoted 66 lines

Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +3 -0

+		interrupts = <29>;
+		clock-frequency = <100000>;
+	};

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

index 550eb77..b6f475c 100644
@@ -112,7 +112,6 @@
 
 			i2c0: i2c at 11000 {
 				compatible = "marvell,mv64xxx-i2c";
-				reg = <0x11000 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				interrupts = <31>;

@@ -123,7 +122,6 @@
 
 			i2c1: i2c at 11100 {
 				compatible = "marvell,mv64xxx-i2c";
-				reg = <0x11100 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				interrupts = <32>;

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

index aee2b18..39b26d6 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 5b902f9..c2c357e 100644
@@ -134,6 +134,16 @@
 				};
 			};
 
+			i2c0: i2c at 11000 {
+				compatible = "marvell,mv64xxx-i2c", "marvell,mv78230-i2c";
+				reg = <0x11000 0x100>;
+			};
+
+			i2c1: i2c at 11100 {
+				compatible = "marvell,mv64xxx-i2c", "marvell,mv78230-i2c";
+				reg = <0x11100 0x100>;
+			};
+
 			usb at 50000 {
 				clocks = <&gateclk 18>;
 			};

Signature · 1 line

1.8.1.2

Sebastian Hesselbarth

2013-06-21 14:15 UTC #9 lore raw

PATCH v3 4/4 ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

Quoted 29 lines · Jason Cooper

Quoted 28 lines · 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 +0 -0
index a1ee681..ce7af6a 100644
@@ -4,7 +4,8 @@
  Required properties :
 

Quoted 4 lines

Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt line 8
Quoted 4 lines
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +2 -1
   - 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

Jason, Gregory,

Also, s/mv7230/mv78230/

from a quick check of the patch set (which you forgot to send to LKML) I am wondering why you didn't update the of matches struct with the new compatible for "marvell,mv78230-i2c"? This will save you from still having "marvell,mv64xxx-i2c" as additional compatible to match device and driver. With that the above should also be s/and/or/.

Quoted 17 lines

Quoted 4 lines
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +0 -0
   - interrupts      : The interrupt number
 
  Optional properties :
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt Examples: · line 30
Quoted 10 lines
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +7 -0
@@ -20,3 +21,13 @@ Examples:
  		interrupts = <29>;
  		clock-frequency = <100000>;
  	};
+
+For a controller which support the I2C Transaction Generator:
+
+	i2c at 11000 {
+		compatible = "marvell,mv64xxx-i2c", "marvell,mv78230-i2c";
+		reg = <0x11000 0x100>;
+		compatible = "marvell,mv64xxx-i2c";
extra compatible line. If there's nothing else, I'll fix this up when I pull it in.

Gregory CLEMENT

author

2013-06-21 14:23 UTC #10 lore raw

PATCH v3 4/4 ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

Quoted 30 lines · Sebastian Hesselbarth

Quoted 29 lines · Jason Cooper
Quoted 28 lines · 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 +0 -0
index a1ee681..ce7af6a 100644
@@ -4,7 +4,8 @@
  Required properties :
 

Quoted 8 lines

Quoted 4 lines
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt line 8
Quoted 4 lines
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +2 -1
   - 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
Jason, Gregory,
Also, s/mv7230/mv78230/

You are right, thanks for both of you to notice it.

Quoted 8 lines

from a quick check of the patch set (which you forgot to send to LKML) I am wondering why you didn't update the of matches struct with the new compatible for "marvell,mv78230-i2c"? This will save you from still having "marvell,mv64xxx-i2c" as additional compatible to match device and driver. With that the above should also be s/and/or/.
From my point of view it is more an extension than an other kind of controller like

the one embedded in the AllWinner SoCs. So I thought it was more in the spirit of the way you describe hardware in the device tree, but I am far for being an expert so I maybe wrong.

Quoted 19 lines

Quoted 18 lines
Quoted 5 lines
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +0 -0
   - interrupts      : The interrupt number
 
  Optional properties :
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt Examples: · line 30
Quoted 10 lines
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +7 -0
@@ -20,3 +21,13 @@ Examples:
  		interrupts = <29>;
  		clock-frequency = <100000>;
  	};
+
+For a controller which support the I2C Transaction Generator:
+
+	i2c at 11000 {
+		compatible = "marvell,mv64xxx-i2c", "marvell,mv78230-i2c";
+		reg = <0x11000 0x100>;
+		compatible = "marvell,mv64xxx-i2c";
extra compatible line. If there's nothing else, I'll fix this up when I pull it in.

Signature · 4 lines

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

Jason Cooper

2013-06-21 14:56 UTC #11 lore raw

PATCH v3 4/4 ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

In reply to Sebastian Hesselbarth

Quoted 30 lines · Sebastian Hesselbarth

Quoted 29 lines · Jason Cooper
Quoted 28 lines · 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 +0 -0
index a1ee681..ce7af6a 100644
@@ -4,7 +4,8 @@
 Required properties :
 

Quoted 8 lines

Quoted 4 lines
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt line 8
Quoted 4 lines
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +2 -1
  - 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
Jason, Gregory,
Also, s/mv7230/mv78230/

noted.

Quoted 5 lines

from a quick check of the patch set (which you forgot to send to LKML) I am wondering why you didn't update the of matches struct with the new compatible for "marvell,mv78230-i2c"? This will save you from still having "marvell,mv64xxx-i2c" as additional compatible to match device and driver. With that the above should also be s/and/or/.

agreed, good point.

Gregory,

I'm sending the last PRs for mvebu today. I'll include this one with my suggestion and Sebastian's if you're ok with it. That means you'll have to respin the series for the i2c folks. Does that work for you?

thx,

Jason.

Quoted 23 lines

Quoted 17 lines
Quoted 4 lines
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +0 -0
  - interrupts      : The interrupt number
 
 Optional properties :
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt Examples: · line 30
Quoted 10 lines
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +7 -0
@@ -20,3 +21,13 @@ Examples:
 		interrupts = <29>;
 		clock-frequency = <100000>;
 	};
+
+For a controller which support the I2C Transaction Generator:
+
+	i2c at 11000 {
+		compatible = "marvell,mv64xxx-i2c", "marvell,mv78230-i2c";
+		reg = <0x11000 0x100>;
+		compatible = "marvell,mv64xxx-i2c";
extra compatible line. If there's nothing else, I'll fix this up when I pull it in.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Wolfram Sang

2013-06-21 15:03 UTC #12 lore raw

PATCH v3 4/4 ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

Quoted 7 lines

Quoted 6 lines
from a quick check of the patch set (which you forgot to send to LKML) I am wondering why you didn't update the of matches struct with the new compatible for "marvell,mv78230-i2c"? This will save you from still having "marvell,mv64xxx-i2c" as additional compatible to match device and driver. With that the above should also be s/and/or/.

Quoted 7 lines

agreed, good point.
Gregory,
I'm sending the last PRs for mvebu today. I'll include this one with my suggestion and Sebastian's if you're ok with it. That means you'll have to respin the series for the i2c folks. Does that work for you?

No hurry, please. Most of the patches are 3.12 material anyhow, so we can as well just wait until the I2C patches are okay and then update the dts later.

Jason Cooper

2013-06-21 15:06 UTC #13 lore raw

PATCH v3 4/4 ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

Quoted 11 lines · Wolfram Sang

Quoted 11 lines
Quoted 6 lines
from a quick check of the patch set (which you forgot to send to LKML) I am wondering why you didn't update the of matches struct with the new compatible for "marvell,mv78230-i2c"? This will save you from still having "marvell,mv64xxx-i2c" as additional compatible to match device and driver. With that the above should also be s/and/or/.
agreed, good point.
Gregory,

Quoted 7 lines

Quoted 3 lines
I'm sending the last PRs for mvebu today. I'll include this one with my suggestion and Sebastian's if you're ok with it. That means you'll have to respin the series for the i2c folks. Does that work for you?
No hurry, please. Most of the patches are 3.12 material anyhow, so we can as well just wait until the I2C patches are okay and then update the dts later.

Ok, thanks for the heads up. I'll put this in my v3.12 stack.

thx,

Jason.

Jason Gunthorpe

2013-06-21 17:18 UTC #14 lore raw

PATCH v3 4/4 ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

In reply to Sebastian Hesselbarth

Quoted 5 lines · Sebastian Hesselbarth

from a quick check of the patch set (which you forgot to send to LKML) I am wondering why you didn't update the of matches struct with the new compatible for "marvell,mv78230-i2c"? This will save you from still having "marvell,mv64xxx-i2c" as additional compatible to match device and driver. With that the above should also be s/and/or/.

Agree, I noticed the same things.

Alos, the compatible string should be:

  compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";

(note the order reversal, most specific is first)

and I think it is better to use the 'data' field of of_device_id than a of_is_compatible call. The former is sensitive to order of the compatible string, the latter is not.

Jason

Sebastian Hesselbarth

2013-06-22 16:14 UTC #15 lore raw

PATCH v3 4/4 ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

Quoted 7 lines · Jason Gunthorpe

Quoted 5 lines · Sebastian Hesselbarth
from a quick check of the patch set (which you forgot to send to LKML) I am wondering why you didn't update the of matches struct with the new compatible for "marvell,mv78230-i2c"? This will save you from still having "marvell,mv64xxx-i2c" as additional compatible to match device and driver. With that the above should also be s/and/or/.

Quoted 7 lines

Agree, I noticed the same things.
Alos, the compatible string should be:
   compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
(note the order reversal, most specific is first)

Actually, I suggest to use "marvell,mv78230-i2c" alone. With a new entry in the match table it will be sufficient for matching driver and device. BTW, is mv78230 sufficient to match all SoC variants having this feature? Maybe mv78xx0 but that could interfere with Discovery Innovation or even better armada-xp-i2c (or armada-370-i2c)?

Quoted 3 lines

and I think it is better to use the 'data' field of of_device_id than a of_is_compatible call. The former is sensitive to order of the compatible string, the latter is not.

IMHO as long as there is only one compatible and only a bool to set, using of_device_is_compatible without data pointer is fine here. IIRC sunxi i2c isn't using data pointer the different register offset struct, so we shouldn't for a simple bool. And casting a bool to (void *) looks awkward.

Sebastian

Gregory CLEMENT

author

2013-06-24 09:54 UTC #16 lore raw

PATCH v3 4/4 ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

Quoted 10 lines · Sebastian Hesselbarth

Quoted 9 lines · Jason Gunthorpe
Quoted 5 lines · Sebastian Hesselbarth
from a quick check of the patch set (which you forgot to send to LKML) I am wondering why you didn't update the of matches struct with the new compatible for "marvell,mv78230-i2c"? This will save you from still having "marvell,mv64xxx-i2c" as additional compatible to match device and driver. With that the above should also be s/and/or/.
Agree, I noticed the same things.

Quoted 8 lines

Quoted 5 lines
Alos, the compatible string should be:
   compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
(note the order reversal, most specific is first)
Actually, I suggest to use "marvell,mv78230-i2c" alone. With a new entry in the match table it will be sufficient for matching driver and

I would prefer using a specialization rather than a new entry, so I can just use the "marvell,mv64xxx-i2c" entry for the data pointer .

Quoted 3 lines

device. BTW, is mv78230 sufficient to match all SoC variants having this feature? Maybe mv78xx0 but that could interfere with Discovery Innovation or even better armada-xp-i2c (or armada-370-i2c)?

I am pretty sure that the convention in the device tree is to always use specific product numbers, rather than wildcards. So, here, the choice of mv78230 was just that we can see the mv78260 and mv78460 as extension of the mv78230, so I chose the smallest common denominator.

Quoted 17 lines

Quoted 4 lines
and I think it is better to use the 'data' field of of_device_id than a of_is_compatible call. The former is sensitive to order of the compatible string, the latter is not.
IMHO as long as there is only one compatible and only a bool to set, using of_device_is_compatible without data pointer is fine here. IIRC sunxi i2c isn't using data pointer the different register offset struct, so we shouldn't for a simple bool. And casting a bool to (void *) looks awkward.
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 4 lines

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