* [PATCH 0/3] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP
@ 2013-07-15 14:24 ` Gregory CLEMENT
0 siblings, 0 replies; 34+ messages in thread
From: Gregory CLEMENT @ 2013-07-15 14:24 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c-u79uwXL29TY76Z2rM5mHXA, Jason Cooper,
Andrew Lunn, Gregory CLEMENT
Cc: Thomas Petazzoni, Ezequiel Garcia, Sebastian Hesselbarth,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Nicolas Pitre,
Lior Amsalem, Maen Suleiman, Tawfik Bayouk, Shadi Ammouri,
Eran Ben-Avi, Yehuda Yitschak, Nadav Haklai, Ike Pan,
Chris Van Hoof, Dan Frazier, Leif Lindholm, Jon Masters,
David Marlin
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 is mainly a updated series rebased on 3.11-rc1.
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 and should go through mvebu subsystem.
Thanks,
Change log:
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 | 216 ++++++++++++++++++++-
5 files changed, 236 insertions(+), 13 deletions(-)
--
1.8.1.2
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 0/3] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP
@ 2013-07-15 14:24 ` Gregory CLEMENT
0 siblings, 0 replies; 34+ messages in thread
From: Gregory CLEMENT @ 2013-07-15 14:24 UTC (permalink / raw)
To: linux-arm-kernel
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 is mainly a updated series rebased on 3.11-rc1.
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 and should go through mvebu subsystem.
Thanks,
Change log:
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 | 216 ++++++++++++++++++++-
5 files changed, 236 insertions(+), 13 deletions(-)
--
1.8.1.2
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
2013-07-15 14:24 ` Gregory CLEMENT
@ 2013-07-15 14:24 ` Gregory CLEMENT
-1 siblings, 0 replies; 34+ messages in thread
From: Gregory CLEMENT @ 2013-07-15 14:24 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c-u79uwXL29TY76Z2rM5mHXA, Jason Cooper,
Andrew Lunn, Gregory CLEMENT
Cc: Thomas Petazzoni, Ezequiel Garcia, Sebastian Hesselbarth,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Nicolas Pitre,
Lior Amsalem, Maen Suleiman, Tawfik Bayouk, Shadi Ammouri,
Eran Ben-Avi, Yehuda Yitschak, Nadav Haklai, Ike Pan,
Chris Van Hoof, Dan Frazier, Leif Lindholm, Jon Masters,
David Marlin, Piotr Ziecik
The I2C Transaction Generator offloads CPU from managing I2C
transfer step by step.
This feature is currently only available on Armada XP, so usage of
this mechanism is activated through device tree.
Based on the work of Piotr Ziecik and rewrote to use the new way of
handling multiples i2c messages.
Signed-off-by: Piotr Ziecik <kosmo-nYOzD4b6Jr9Wk0Htik3J/w@public.gmane.org>
Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
drivers/i2c/busses/i2c-mv64xxx.c | 207 ++++++++++++++++++++++++++++++++++++---
1 file changed, 196 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index b1f42bf..355ab64 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -55,6 +55,32 @@
#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK 0xe8
#define MV64XXX_I2C_STATUS_NO_STATUS 0xf8
+/* Register defines (I2C bridge) */
+#define MV64XXX_I2C_REG_TX_DATA_LO 0xc0
+#define MV64XXX_I2C_REG_TX_DATA_HI 0xc4
+#define MV64XXX_I2C_REG_RX_DATA_LO 0xc8
+#define MV64XXX_I2C_REG_RX_DATA_HI 0xcc
+#define MV64XXX_I2C_REG_BRIDGE_CONTROL 0xd0
+#define MV64XXX_I2C_REG_BRIDGE_STATUS 0xd4
+#define MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE 0xd8
+#define MV64XXX_I2C_REG_BRIDGE_INTR_MASK 0xdC
+#define MV64XXX_I2C_REG_BRIDGE_TIMING 0xe0
+
+/* Bridge Control values */
+#define MV64XXX_I2C_BRIDGE_CONTROL_WR 0x00000001
+#define MV64XXX_I2C_BRIDGE_CONTROL_RD 0x00000002
+#define MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT 2
+#define MV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT 0x00001000
+#define MV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT 13
+#define MV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT 16
+#define MV64XXX_I2C_BRIDGE_CONTROL_ENABLE 0x00080000
+
+/* Bridge Status values */
+#define MV64XXX_I2C_BRIDGE_STATUS_ERROR 0x00000001
+#define MV64XXX_I2C_STATUS_OFFLOAD_ERROR 0xf0000001
+#define MV64XXX_I2C_STATUS_OFFLOAD_OK 0xf0000000
+
+
/* Driver states */
enum {
MV64XXX_I2C_STATE_INVALID,
@@ -71,14 +97,17 @@ enum {
enum {
MV64XXX_I2C_ACTION_INVALID,
MV64XXX_I2C_ACTION_CONTINUE,
+ MV64XXX_I2C_ACTION_OFFLOAD_SEND_START,
MV64XXX_I2C_ACTION_SEND_START,
MV64XXX_I2C_ACTION_SEND_RESTART,
+ MV64XXX_I2C_ACTION_OFFLOAD_RESTART,
MV64XXX_I2C_ACTION_SEND_ADDR_1,
MV64XXX_I2C_ACTION_SEND_ADDR_2,
MV64XXX_I2C_ACTION_SEND_DATA,
MV64XXX_I2C_ACTION_RCV_DATA,
MV64XXX_I2C_ACTION_RCV_DATA_STOP,
MV64XXX_I2C_ACTION_SEND_STOP,
+ MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP,
};
struct mv64xxx_i2c_regs {
@@ -117,6 +146,7 @@ struct mv64xxx_i2c_data {
spinlock_t lock;
struct i2c_msg *msg;
struct i2c_adapter adapter;
+ bool offload_enabled;
};
static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
@@ -165,6 +195,79 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
}
}
+static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
+{
+ unsigned long data_reg_hi = 0;
+ unsigned long data_reg_lo = 0;
+ unsigned long ctrl_reg;
+ unsigned int i;
+ struct i2c_msg *msg = drv_data->msgs;
+
+ drv_data->msg = msg;
+ drv_data->byte_posn = 0;
+ drv_data->bytes_left = msg->len;
+ drv_data->aborting = 0;
+ drv_data->rc = 0;
+ /* Only regular transactions can be offloaded */
+ if ((msg->flags & ~(I2C_M_TEN | I2C_M_RD)) != 0)
+ return 1;
+
+ /* Only 1-8 byte transfers can be offloaded */
+ if (msg->len < 1 || msg->len > 8)
+ return 1;
+
+ /* Build transaction */
+ ctrl_reg = MV64XXX_I2C_BRIDGE_CONTROL_ENABLE |
+ (msg->addr << MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT);
+
+ if ((msg->flags & I2C_M_TEN) != 0)
+ ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT;
+
+ if ((msg->flags & I2C_M_RD) == 0) {
+ for (i = 0; i < 4 && i < msg->len; i++)
+ data_reg_lo = data_reg_lo |
+ (msg->buf[i] << ((i & 0x3) * 8));
+
+ for (i = 4; i < 8 && i < msg->len; i++)
+ data_reg_hi = data_reg_hi |
+ (msg->buf[i] << ((i & 0x3) * 8));
+
+ ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_WR |
+ (msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT;
+ } else {
+ ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_RD |
+ (msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT;
+ }
+
+ /* Execute transaction */
+ writel_relaxed(data_reg_lo,
+ drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_LO);
+ writel_relaxed(data_reg_hi,
+ drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_HI);
+ writel(ctrl_reg, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
+
+ return 0;
+}
+
+static void
+mv64xxx_i2c_update_offload_data(struct i2c_msg *msg, unsigned long data_reg_hi,
+ unsigned long data_reg_lo)
+{
+ int i;
+
+ if ((msg->flags & I2C_M_RD) != 0) {
+ for (i = 0; i < 4 && i < msg->len; i++) {
+ msg->buf[i] = data_reg_lo & 0xFF;
+ data_reg_lo >>= 8;
+ }
+
+ for (i = 4; i < 8 && i < msg->len; i++) {
+ msg->buf[i] = data_reg_hi & 0xFF;
+ data_reg_hi >>= 8;
+ }
+ }
+
+}
/*
*****************************************************************************
*
@@ -177,6 +280,15 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
static void
mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
{
+ if (drv_data->offload_enabled) {
+ writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
+ writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_TIMING);
+ writel(0, drv_data->reg_base +
+ MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
+ writel(0, drv_data->reg_base +
+ MV64XXX_I2C_REG_BRIDGE_INTR_MASK);
+ }
+
writel(0, drv_data->reg_base + drv_data->reg_offsets.soft_reset);
writel(MV64XXX_I2C_BAUD_DIV_M(drv_data->freq_m) | MV64XXX_I2C_BAUD_DIV_N(drv_data->freq_n),
drv_data->reg_base + drv_data->reg_offsets.clock);
@@ -283,6 +395,16 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
drv_data->rc = -ENXIO;
break;
+ case MV64XXX_I2C_STATUS_OFFLOAD_OK:
+ if (drv_data->send_stop || drv_data->aborting) {
+ drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP;
+ drv_data->state = MV64XXX_I2C_STATE_IDLE;
+ } else {
+ drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_RESTART;
+ drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_RESTART;
+ }
+ break;
+
default:
dev_err(&drv_data->adapter.dev,
"mv64xxx_i2c_fsm: Ctlr Error -- state: 0x%x, "
@@ -298,21 +420,36 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
static void
mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
{
+ unsigned long data_reg_hi = 0;
+ unsigned long data_reg_lo = 0;
+
switch(drv_data->action) {
+ case MV64XXX_I2C_ACTION_OFFLOAD_RESTART:
+ data_reg_lo = readl(drv_data->reg_base +
+ MV64XXX_I2C_REG_RX_DATA_LO);
+ data_reg_hi = readl(drv_data->reg_base +
+ MV64XXX_I2C_REG_RX_DATA_HI);
+ mv64xxx_i2c_update_offload_data(drv_data->msg, data_reg_hi,
+ data_reg_lo);
+ writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
+ writel(0, drv_data->reg_base +
+ MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
+ /* FALLTHRU */
case MV64XXX_I2C_ACTION_SEND_RESTART:
/* We should only get here if we have further messages */
BUG_ON(drv_data->num_msgs == 0);
- drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
- writel(drv_data->cntl_bits,
- drv_data->reg_base + drv_data->reg_offsets.control);
-
drv_data->msgs++;
drv_data->num_msgs--;
+ if (!(drv_data->offload_enabled &&
+ mv64xxx_i2c_offload_msg(drv_data))) {
+ drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
+ writel(drv_data->cntl_bits,
+ drv_data->reg_base + drv_data->reg_offsets.control);
- /* Setup for the next message */
- mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
-
+ /* Setup for the next message */
+ mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+ }
/*
* We're never at the start of the message here, and by this
* time it's already too late to do any protocol mangling.
@@ -326,6 +463,12 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
drv_data->reg_base + drv_data->reg_offsets.control);
break;
+ case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
+ if (mv64xxx_i2c_offload_msg(drv_data) <= 0)
+ break;
+ else
+ drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+ /* FALLTHRU */
case MV64XXX_I2C_ACTION_SEND_START:
writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
drv_data->reg_base + drv_data->reg_offsets.control);
@@ -375,6 +518,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
"mv64xxx_i2c_do_action: Invalid action: %d\n",
drv_data->action);
drv_data->rc = -EIO;
+
/* FALLTHRU */
case MV64XXX_I2C_ACTION_SEND_STOP:
drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
@@ -383,6 +527,20 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
drv_data->block = 0;
wake_up(&drv_data->waitq);
break;
+
+ case MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP:
+ data_reg_lo = readl(drv_data->reg_base +
+ MV64XXX_I2C_REG_RX_DATA_LO);
+ data_reg_hi = readl(drv_data->reg_base +
+ MV64XXX_I2C_REG_RX_DATA_HI);
+ mv64xxx_i2c_update_offload_data(drv_data->msg, data_reg_hi,
+ data_reg_lo);
+ writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
+ writel(0, drv_data->reg_base +
+ MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
+ drv_data->block = 0;
+ wake_up(&drv_data->waitq);
+ break;
}
}
@@ -395,6 +553,21 @@ mv64xxx_i2c_intr(int irq, void *dev_id)
irqreturn_t rc = IRQ_NONE;
spin_lock_irqsave(&drv_data->lock, flags);
+
+ if (drv_data->offload_enabled) {
+ while (readl(drv_data->reg_base +
+ MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE)) {
+ int reg_status = readl(drv_data->reg_base +
+ MV64XXX_I2C_REG_BRIDGE_STATUS);
+ if (reg_status & MV64XXX_I2C_BRIDGE_STATUS_ERROR)
+ status = MV64XXX_I2C_STATUS_OFFLOAD_ERROR;
+ else
+ status = MV64XXX_I2C_STATUS_OFFLOAD_OK;
+ mv64xxx_i2c_fsm(drv_data, status);
+ mv64xxx_i2c_do_action(drv_data);
+ rc = IRQ_HANDLED;
+ }
+ }
while (readl(drv_data->reg_base + drv_data->reg_offsets.control) &
MV64XXX_I2C_REG_CONTROL_IFLG) {
status = readl(drv_data->reg_base + drv_data->reg_offsets.status);
@@ -459,11 +632,15 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
unsigned long flags;
spin_lock_irqsave(&drv_data->lock, flags);
- mv64xxx_i2c_prepare_for_io(drv_data, msg);
-
- drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
- drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+ if (drv_data->offload_enabled) {
+ drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_SEND_START;
+ drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+ } else {
+ mv64xxx_i2c_prepare_for_io(drv_data, msg);
+ drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+ drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+ }
drv_data->send_stop = is_last;
drv_data->block = 1;
mv64xxx_i2c_do_action(drv_data);
@@ -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);
--
1.8.1.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
@ 2013-07-15 14:24 ` Gregory CLEMENT
0 siblings, 0 replies; 34+ messages in thread
From: Gregory CLEMENT @ 2013-07-15 14:24 UTC (permalink / raw)
To: linux-arm-kernel
The I2C Transaction Generator offloads CPU from managing I2C
transfer step by step.
This feature is currently only available on Armada XP, so usage of
this mechanism is activated through device tree.
Based on the work of Piotr Ziecik and rewrote to use the new way of
handling multiples i2c messages.
Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
drivers/i2c/busses/i2c-mv64xxx.c | 207 ++++++++++++++++++++++++++++++++++++---
1 file changed, 196 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index b1f42bf..355ab64 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -55,6 +55,32 @@
#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK 0xe8
#define MV64XXX_I2C_STATUS_NO_STATUS 0xf8
+/* Register defines (I2C bridge) */
+#define MV64XXX_I2C_REG_TX_DATA_LO 0xc0
+#define MV64XXX_I2C_REG_TX_DATA_HI 0xc4
+#define MV64XXX_I2C_REG_RX_DATA_LO 0xc8
+#define MV64XXX_I2C_REG_RX_DATA_HI 0xcc
+#define MV64XXX_I2C_REG_BRIDGE_CONTROL 0xd0
+#define MV64XXX_I2C_REG_BRIDGE_STATUS 0xd4
+#define MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE 0xd8
+#define MV64XXX_I2C_REG_BRIDGE_INTR_MASK 0xdC
+#define MV64XXX_I2C_REG_BRIDGE_TIMING 0xe0
+
+/* Bridge Control values */
+#define MV64XXX_I2C_BRIDGE_CONTROL_WR 0x00000001
+#define MV64XXX_I2C_BRIDGE_CONTROL_RD 0x00000002
+#define MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT 2
+#define MV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT 0x00001000
+#define MV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT 13
+#define MV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT 16
+#define MV64XXX_I2C_BRIDGE_CONTROL_ENABLE 0x00080000
+
+/* Bridge Status values */
+#define MV64XXX_I2C_BRIDGE_STATUS_ERROR 0x00000001
+#define MV64XXX_I2C_STATUS_OFFLOAD_ERROR 0xf0000001
+#define MV64XXX_I2C_STATUS_OFFLOAD_OK 0xf0000000
+
+
/* Driver states */
enum {
MV64XXX_I2C_STATE_INVALID,
@@ -71,14 +97,17 @@ enum {
enum {
MV64XXX_I2C_ACTION_INVALID,
MV64XXX_I2C_ACTION_CONTINUE,
+ MV64XXX_I2C_ACTION_OFFLOAD_SEND_START,
MV64XXX_I2C_ACTION_SEND_START,
MV64XXX_I2C_ACTION_SEND_RESTART,
+ MV64XXX_I2C_ACTION_OFFLOAD_RESTART,
MV64XXX_I2C_ACTION_SEND_ADDR_1,
MV64XXX_I2C_ACTION_SEND_ADDR_2,
MV64XXX_I2C_ACTION_SEND_DATA,
MV64XXX_I2C_ACTION_RCV_DATA,
MV64XXX_I2C_ACTION_RCV_DATA_STOP,
MV64XXX_I2C_ACTION_SEND_STOP,
+ MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP,
};
struct mv64xxx_i2c_regs {
@@ -117,6 +146,7 @@ struct mv64xxx_i2c_data {
spinlock_t lock;
struct i2c_msg *msg;
struct i2c_adapter adapter;
+ bool offload_enabled;
};
static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
@@ -165,6 +195,79 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
}
}
+static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
+{
+ unsigned long data_reg_hi = 0;
+ unsigned long data_reg_lo = 0;
+ unsigned long ctrl_reg;
+ unsigned int i;
+ struct i2c_msg *msg = drv_data->msgs;
+
+ drv_data->msg = msg;
+ drv_data->byte_posn = 0;
+ drv_data->bytes_left = msg->len;
+ drv_data->aborting = 0;
+ drv_data->rc = 0;
+ /* Only regular transactions can be offloaded */
+ if ((msg->flags & ~(I2C_M_TEN | I2C_M_RD)) != 0)
+ return 1;
+
+ /* Only 1-8 byte transfers can be offloaded */
+ if (msg->len < 1 || msg->len > 8)
+ return 1;
+
+ /* Build transaction */
+ ctrl_reg = MV64XXX_I2C_BRIDGE_CONTROL_ENABLE |
+ (msg->addr << MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT);
+
+ if ((msg->flags & I2C_M_TEN) != 0)
+ ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT;
+
+ if ((msg->flags & I2C_M_RD) == 0) {
+ for (i = 0; i < 4 && i < msg->len; i++)
+ data_reg_lo = data_reg_lo |
+ (msg->buf[i] << ((i & 0x3) * 8));
+
+ for (i = 4; i < 8 && i < msg->len; i++)
+ data_reg_hi = data_reg_hi |
+ (msg->buf[i] << ((i & 0x3) * 8));
+
+ ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_WR |
+ (msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT;
+ } else {
+ ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_RD |
+ (msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT;
+ }
+
+ /* Execute transaction */
+ writel_relaxed(data_reg_lo,
+ drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_LO);
+ writel_relaxed(data_reg_hi,
+ drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_HI);
+ writel(ctrl_reg, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
+
+ return 0;
+}
+
+static void
+mv64xxx_i2c_update_offload_data(struct i2c_msg *msg, unsigned long data_reg_hi,
+ unsigned long data_reg_lo)
+{
+ int i;
+
+ if ((msg->flags & I2C_M_RD) != 0) {
+ for (i = 0; i < 4 && i < msg->len; i++) {
+ msg->buf[i] = data_reg_lo & 0xFF;
+ data_reg_lo >>= 8;
+ }
+
+ for (i = 4; i < 8 && i < msg->len; i++) {
+ msg->buf[i] = data_reg_hi & 0xFF;
+ data_reg_hi >>= 8;
+ }
+ }
+
+}
/*
*****************************************************************************
*
@@ -177,6 +280,15 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
static void
mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
{
+ if (drv_data->offload_enabled) {
+ writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
+ writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_TIMING);
+ writel(0, drv_data->reg_base +
+ MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
+ writel(0, drv_data->reg_base +
+ MV64XXX_I2C_REG_BRIDGE_INTR_MASK);
+ }
+
writel(0, drv_data->reg_base + drv_data->reg_offsets.soft_reset);
writel(MV64XXX_I2C_BAUD_DIV_M(drv_data->freq_m) | MV64XXX_I2C_BAUD_DIV_N(drv_data->freq_n),
drv_data->reg_base + drv_data->reg_offsets.clock);
@@ -283,6 +395,16 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
drv_data->rc = -ENXIO;
break;
+ case MV64XXX_I2C_STATUS_OFFLOAD_OK:
+ if (drv_data->send_stop || drv_data->aborting) {
+ drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP;
+ drv_data->state = MV64XXX_I2C_STATE_IDLE;
+ } else {
+ drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_RESTART;
+ drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_RESTART;
+ }
+ break;
+
default:
dev_err(&drv_data->adapter.dev,
"mv64xxx_i2c_fsm: Ctlr Error -- state: 0x%x, "
@@ -298,21 +420,36 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
static void
mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
{
+ unsigned long data_reg_hi = 0;
+ unsigned long data_reg_lo = 0;
+
switch(drv_data->action) {
+ case MV64XXX_I2C_ACTION_OFFLOAD_RESTART:
+ data_reg_lo = readl(drv_data->reg_base +
+ MV64XXX_I2C_REG_RX_DATA_LO);
+ data_reg_hi = readl(drv_data->reg_base +
+ MV64XXX_I2C_REG_RX_DATA_HI);
+ mv64xxx_i2c_update_offload_data(drv_data->msg, data_reg_hi,
+ data_reg_lo);
+ writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
+ writel(0, drv_data->reg_base +
+ MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
+ /* FALLTHRU */
case MV64XXX_I2C_ACTION_SEND_RESTART:
/* We should only get here if we have further messages */
BUG_ON(drv_data->num_msgs == 0);
- drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
- writel(drv_data->cntl_bits,
- drv_data->reg_base + drv_data->reg_offsets.control);
-
drv_data->msgs++;
drv_data->num_msgs--;
+ if (!(drv_data->offload_enabled &&
+ mv64xxx_i2c_offload_msg(drv_data))) {
+ drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
+ writel(drv_data->cntl_bits,
+ drv_data->reg_base + drv_data->reg_offsets.control);
- /* Setup for the next message */
- mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
-
+ /* Setup for the next message */
+ mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+ }
/*
* We're never at the start of the message here, and by this
* time it's already too late to do any protocol mangling.
@@ -326,6 +463,12 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
drv_data->reg_base + drv_data->reg_offsets.control);
break;
+ case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
+ if (mv64xxx_i2c_offload_msg(drv_data) <= 0)
+ break;
+ else
+ drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+ /* FALLTHRU */
case MV64XXX_I2C_ACTION_SEND_START:
writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
drv_data->reg_base + drv_data->reg_offsets.control);
@@ -375,6 +518,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
"mv64xxx_i2c_do_action: Invalid action: %d\n",
drv_data->action);
drv_data->rc = -EIO;
+
/* FALLTHRU */
case MV64XXX_I2C_ACTION_SEND_STOP:
drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
@@ -383,6 +527,20 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
drv_data->block = 0;
wake_up(&drv_data->waitq);
break;
+
+ case MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP:
+ data_reg_lo = readl(drv_data->reg_base +
+ MV64XXX_I2C_REG_RX_DATA_LO);
+ data_reg_hi = readl(drv_data->reg_base +
+ MV64XXX_I2C_REG_RX_DATA_HI);
+ mv64xxx_i2c_update_offload_data(drv_data->msg, data_reg_hi,
+ data_reg_lo);
+ writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
+ writel(0, drv_data->reg_base +
+ MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
+ drv_data->block = 0;
+ wake_up(&drv_data->waitq);
+ break;
}
}
@@ -395,6 +553,21 @@ mv64xxx_i2c_intr(int irq, void *dev_id)
irqreturn_t rc = IRQ_NONE;
spin_lock_irqsave(&drv_data->lock, flags);
+
+ if (drv_data->offload_enabled) {
+ while (readl(drv_data->reg_base +
+ MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE)) {
+ int reg_status = readl(drv_data->reg_base +
+ MV64XXX_I2C_REG_BRIDGE_STATUS);
+ if (reg_status & MV64XXX_I2C_BRIDGE_STATUS_ERROR)
+ status = MV64XXX_I2C_STATUS_OFFLOAD_ERROR;
+ else
+ status = MV64XXX_I2C_STATUS_OFFLOAD_OK;
+ mv64xxx_i2c_fsm(drv_data, status);
+ mv64xxx_i2c_do_action(drv_data);
+ rc = IRQ_HANDLED;
+ }
+ }
while (readl(drv_data->reg_base + drv_data->reg_offsets.control) &
MV64XXX_I2C_REG_CONTROL_IFLG) {
status = readl(drv_data->reg_base + drv_data->reg_offsets.status);
@@ -459,11 +632,15 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
unsigned long flags;
spin_lock_irqsave(&drv_data->lock, flags);
- mv64xxx_i2c_prepare_for_io(drv_data, msg);
-
- drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
- drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+ if (drv_data->offload_enabled) {
+ drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_SEND_START;
+ drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+ } else {
+ mv64xxx_i2c_prepare_for_io(drv_data, msg);
+ drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+ drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+ }
drv_data->send_stop = is_last;
drv_data->block = 1;
mv64xxx_i2c_do_action(drv_data);
@@ -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);
--
1.8.1.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 2/3] i2c-mv64xxx: Fix timing issue on Armada XP (errata FE-8471889)
2013-07-15 14:24 ` Gregory CLEMENT
@ 2013-07-15 14:24 ` Gregory CLEMENT
-1 siblings, 0 replies; 34+ messages in thread
From: Gregory CLEMENT @ 2013-07-15 14:24 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c-u79uwXL29TY76Z2rM5mHXA, Jason Cooper,
Andrew Lunn, Gregory CLEMENT
Cc: Thomas Petazzoni, Ezequiel Garcia, Sebastian Hesselbarth,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Nicolas Pitre,
Lior Amsalem, Maen Suleiman, Tawfik Bayouk, Shadi Ammouri,
Eran Ben-Avi, Yehuda Yitschak, Nadav Haklai, Ike Pan,
Chris Van Hoof, Dan Frazier, Leif Lindholm, Jon Masters,
David Marlin, Zbigniew Bodek
All the Armada XP (mv78230, mv78260 and mv78460) have a silicon issue
in the I2C controller which violate the i2c repeated start
timing. The I2C standard requires a minimum of 4.7us for the repeated
start condition whereas the I2C controller of the Armada XP this time
is 2.9us.
So this patch adds a 5us delay for the start case only if the
the compatible i2c-mv78230 is set.
Based on the initals patches from Zbigniew Bodek
Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Signed-off-by: Zbigniew Bodek <zbb-nYOzD4b6Jr9Wk0Htik3J/w@public.gmane.org>
---
drivers/i2c/busses/i2c-mv64xxx.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 355ab64..75be30cce 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -24,6 +24,7 @@
#include <linux/of_i2c.h>
#include <linux/clk.h>
#include <linux/err.h>
+#include <linux/delay.h>
#define MV64XXX_I2C_ADDR_ADDR(val) ((val & 0x7f) << 1)
#define MV64XXX_I2C_BAUD_DIV_N(val) (val & 0x7)
@@ -147,6 +148,8 @@ struct mv64xxx_i2c_data {
struct i2c_msg *msg;
struct i2c_adapter adapter;
bool offload_enabled;
+/* 5us delay in order to avoid repeated start timing violation */
+ bool errata_delay;
};
static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
@@ -450,6 +453,9 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
/* Setup for the next message */
mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
}
+ if (drv_data->errata_delay)
+ udelay(5);
+
/*
* We're never at the start of the message here, and by this
* time it's already too late to do any protocol mangling.
@@ -509,6 +515,9 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
drv_data->reg_base + drv_data->reg_offsets.control);
drv_data->block = 0;
+ if (drv_data->errata_delay)
+ udelay(5);
+
wake_up(&drv_data->waitq);
break;
@@ -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;
--
1.8.1.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 2/3] i2c-mv64xxx: Fix timing issue on Armada XP (errata FE-8471889)
@ 2013-07-15 14:24 ` Gregory CLEMENT
0 siblings, 0 replies; 34+ messages in thread
From: Gregory CLEMENT @ 2013-07-15 14:24 UTC (permalink / raw)
To: linux-arm-kernel
All the Armada XP (mv78230, mv78260 and mv78460) have a silicon issue
in the I2C controller which violate the i2c repeated start
timing. The I2C standard requires a minimum of 4.7us for the repeated
start condition whereas the I2C controller of the Armada XP this time
is 2.9us.
So this patch adds a 5us delay for the start case only if the
the compatible i2c-mv78230 is set.
Based on the initals patches from Zbigniew Bodek
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Signed-off-by: Zbigniew Bodek <zbb@semihalf.com>
---
drivers/i2c/busses/i2c-mv64xxx.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 355ab64..75be30cce 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -24,6 +24,7 @@
#include <linux/of_i2c.h>
#include <linux/clk.h>
#include <linux/err.h>
+#include <linux/delay.h>
#define MV64XXX_I2C_ADDR_ADDR(val) ((val & 0x7f) << 1)
#define MV64XXX_I2C_BAUD_DIV_N(val) (val & 0x7)
@@ -147,6 +148,8 @@ struct mv64xxx_i2c_data {
struct i2c_msg *msg;
struct i2c_adapter adapter;
bool offload_enabled;
+/* 5us delay in order to avoid repeated start timing violation */
+ bool errata_delay;
};
static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
@@ -450,6 +453,9 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
/* Setup for the next message */
mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
}
+ if (drv_data->errata_delay)
+ udelay(5);
+
/*
* We're never at the start of the message here, and by this
* time it's already too late to do any protocol mangling.
@@ -509,6 +515,9 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
drv_data->reg_base + drv_data->reg_offsets.control);
drv_data->block = 0;
+ if (drv_data->errata_delay)
+ udelay(5);
+
wake_up(&drv_data->waitq);
break;
@@ -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;
--
1.8.1.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 3/3] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c
2013-07-15 14:24 ` Gregory CLEMENT
@ 2013-07-15 14:24 ` Gregory CLEMENT
-1 siblings, 0 replies; 34+ messages in thread
From: Gregory CLEMENT @ 2013-07-15 14:24 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c-u79uwXL29TY76Z2rM5mHXA, Jason Cooper,
Andrew Lunn, Gregory CLEMENT
Cc: Thomas Petazzoni, Ezequiel Garcia, Sebastian Hesselbarth,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Nicolas Pitre,
Lior Amsalem, Maen Suleiman, Tawfik Bayouk, Shadi Ammouri,
Eran Ben-Avi, Yehuda Yitschak, Nadav Haklai, Ike Pan,
Chris Van Hoof, Dan Frazier, Leif Lindholm, Jon Masters,
David Marlin
The mv64xxx-i2c embedded in the Armada XP have a new feature to
offload i2c transaction. This new version of the IP come also with
some errata. This lead to the introduction to a another compatible
string.
This commit split the i2c information into armada-370.dtsi and
armada-xp.dtsi. Most of the data remains the same and stay in the
common file Armada-370-xp.dtsi. With this new feature the size of the
registers are bigger for Armada XP and the new compatible string is
used.
The Device Tree binding documentation is updated accordingly.
Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 13 ++++++++++++-
arch/arm/boot/dts/armada-370-xp.dtsi | 2 --
arch/arm/boot/dts/armada-370.dtsi | 8 ++++++++
arch/arm/boot/dts/armada-xp.dtsi | 10 ++++++++++
4 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
index a1ee681..c5dd952 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
@@ -4,7 +4,8 @@
Required properties :
- reg : Offset and length of the register set for the device
- - compatible : Should be "marvell,mv64xxx-i2c"
+ - compatible : Should be "marvell,mv64xxx-i2c" and "marvell,mv7230-i2c"
+for controller which support the I2C Transaction Generator
- interrupts : The interrupt number
Optional properties :
@@ -20,3 +21,13 @@ Examples:
interrupts = <29>;
clock-frequency = <100000>;
};
+
+For a controller which support the I2C Transaction Generator:
+
+ i2c@11000 {
+ compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
+ reg = <0x11000 0x100>;
+ compatible = "marvell,mv64xxx-i2c";
+ interrupts = <29>;
+ clock-frequency = <100000>;
+ };
diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 90b1176..d8b24c9 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -121,7 +121,6 @@
i2c0: i2c@11000 {
compatible = "marvell,mv64xxx-i2c";
- reg = <0x11000 0x20>;
#address-cells = <1>;
#size-cells = <0>;
interrupts = <31>;
@@ -132,7 +131,6 @@
i2c1: i2c@11100 {
compatible = "marvell,mv64xxx-i2c";
- reg = <0x11100 0x20>;
#address-cells = <1>;
#size-cells = <0>;
interrupts = <32>;
diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index fa3dfc6..0e2eefa 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -155,6 +155,14 @@
};
};
+ i2c0: i2c@11000 {
+ reg = <0x11000 0x20>;
+ };
+
+ i2c1: i2c@11100 {
+ reg = <0x11100 0x20>;
+ };
+
usb@50000 {
clocks = <&coreclk 0>;
};
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index 416eb94..e1f2547 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -138,6 +138,16 @@
};
};
+ i2c0: i2c@11000 {
+ compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
+ reg = <0x11000 0x100>;
+ };
+
+ i2c1: i2c@11100 {
+ compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
+ reg = <0x11100 0x100>;
+ };
+
usb@50000 {
clocks = <&gateclk 18>;
};
--
1.8.1.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 3/3] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c
@ 2013-07-15 14:24 ` Gregory CLEMENT
0 siblings, 0 replies; 34+ messages in thread
From: Gregory CLEMENT @ 2013-07-15 14:24 UTC (permalink / raw)
To: linux-arm-kernel
The mv64xxx-i2c embedded in the Armada XP have a new feature to
offload i2c transaction. This new version of the IP come also with
some errata. This lead to the introduction to a another compatible
string.
This commit split the i2c information into armada-370.dtsi and
armada-xp.dtsi. Most of the data remains the same and stay in the
common file Armada-370-xp.dtsi. With this new feature the size of the
registers are bigger for Armada XP and the new compatible string is
used.
The Device Tree binding documentation is updated accordingly.
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 13 ++++++++++++-
arch/arm/boot/dts/armada-370-xp.dtsi | 2 --
arch/arm/boot/dts/armada-370.dtsi | 8 ++++++++
arch/arm/boot/dts/armada-xp.dtsi | 10 ++++++++++
4 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
index a1ee681..c5dd952 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
@@ -4,7 +4,8 @@
Required properties :
- reg : Offset and length of the register set for the device
- - compatible : Should be "marvell,mv64xxx-i2c"
+ - compatible : Should be "marvell,mv64xxx-i2c" and "marvell,mv7230-i2c"
+for controller which support the I2C Transaction Generator
- interrupts : The interrupt number
Optional properties :
@@ -20,3 +21,13 @@ Examples:
interrupts = <29>;
clock-frequency = <100000>;
};
+
+For a controller which support the I2C Transaction Generator:
+
+ i2c at 11000 {
+ compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
+ reg = <0x11000 0x100>;
+ compatible = "marvell,mv64xxx-i2c";
+ interrupts = <29>;
+ clock-frequency = <100000>;
+ };
diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 90b1176..d8b24c9 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -121,7 +121,6 @@
i2c0: i2c at 11000 {
compatible = "marvell,mv64xxx-i2c";
- reg = <0x11000 0x20>;
#address-cells = <1>;
#size-cells = <0>;
interrupts = <31>;
@@ -132,7 +131,6 @@
i2c1: i2c at 11100 {
compatible = "marvell,mv64xxx-i2c";
- reg = <0x11100 0x20>;
#address-cells = <1>;
#size-cells = <0>;
interrupts = <32>;
diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index fa3dfc6..0e2eefa 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -155,6 +155,14 @@
};
};
+ i2c0: i2c at 11000 {
+ reg = <0x11000 0x20>;
+ };
+
+ i2c1: i2c at 11100 {
+ reg = <0x11100 0x20>;
+ };
+
usb at 50000 {
clocks = <&coreclk 0>;
};
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index 416eb94..e1f2547 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -138,6 +138,16 @@
};
};
+ i2c0: i2c at 11000 {
+ compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
+ reg = <0x11000 0x100>;
+ };
+
+ i2c1: i2c at 11100 {
+ compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
+ reg = <0x11100 0x100>;
+ };
+
usb at 50000 {
clocks = <&gateclk 18>;
};
--
1.8.1.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
2013-07-15 14:24 ` Gregory CLEMENT
@ 2013-07-16 8:05 ` Maxime Ripard
-1 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2013-07-16 8:05 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Wolfram Sang, linux-i2c-u79uwXL29TY76Z2rM5mHXA, Jason Cooper,
Andrew Lunn, Thomas Petazzoni, Lior Amsalem, Yehuda Yitschak,
Ike Pan, Piotr Ziecik, Tawfik Bayouk, Nicolas Pitre, Dan Frazier,
Chris Van Hoof, David Marlin, Eran Ben-Avi, Nadav Haklai,
Maen Suleiman, Shadi Ammouri, Ezequiel Garcia, Jon Masters,
Leif Lindholm, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Sebastian Hesselbarth
[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]
Hi Gregory,
On Mon, Jul 15, 2013 at 04:24:36PM +0200, Gregory CLEMENT wrote:
> The I2C Transaction Generator offloads CPU from managing I2C
> transfer step by step.
>
> This feature is currently only available on Armada XP, so usage of
> this mechanism is activated through device tree.
>
> Based on the work of Piotr Ziecik and rewrote to use the new way of
> handling multiples i2c messages.
>
> Signed-off-by: Piotr Ziecik <kosmo-nYOzD4b6Jr9Wk0Htik3J/w@public.gmane.org>
> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> drivers/i2c/busses/i2c-mv64xxx.c | 207 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 196 insertions(+), 11 deletions(-)
[...]
> + /*
> + * 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;
> +
Do you have a reason for not adding it to the match table? I mean, you
will introduce a new compatible here, but if that compatible is used
alone, won't probe the driver? That doesn't seem very right to me.
Also, you should probably add it to the bindings documentation.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
@ 2013-07-16 8:05 ` Maxime Ripard
0 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2013-07-16 8:05 UTC (permalink / raw)
To: linux-arm-kernel
Hi Gregory,
On Mon, Jul 15, 2013 at 04:24:36PM +0200, Gregory CLEMENT wrote:
> The I2C Transaction Generator offloads CPU from managing I2C
> transfer step by step.
>
> This feature is currently only available on Armada XP, so usage of
> this mechanism is activated through device tree.
>
> Based on the work of Piotr Ziecik and rewrote to use the new way of
> handling multiples i2c messages.
>
> Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
> drivers/i2c/busses/i2c-mv64xxx.c | 207 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 196 insertions(+), 11 deletions(-)
[...]
> + /*
> + * 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;
> +
Do you have a reason for not adding it to the match table? I mean, you
will introduce a new compatible here, but if that compatible is used
alone, won't probe the driver? That doesn't seem very right to me.
Also, you should probably add it to the bindings documentation.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- 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/20130716/512ddec5/attachment.sig>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c
2013-07-15 14:24 ` Gregory CLEMENT
@ 2013-08-03 17:36 ` Jason Cooper
-1 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2013-08-03 17:36 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Wolfram Sang, linux-i2c-u79uwXL29TY76Z2rM5mHXA, Andrew Lunn,
Thomas Petazzoni, Lior Amsalem, Yehuda Yitschak, Ike Pan,
Tawfik Bayouk, Nicolas Pitre, Dan Frazier, Chris Van Hoof,
David Marlin, Eran Ben-Avi, Nadav Haklai, Maen Suleiman,
Shadi Ammouri, Ezequiel Garcia, Jon Masters, Leif Lindholm,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Sebastian Hesselbarth
On Mon, Jul 15, 2013 at 04:24:38PM +0200, Gregory CLEMENT wrote:
> The mv64xxx-i2c embedded in the Armada XP have a new feature to
> offload i2c transaction. This new version of the IP come also with
> some errata. This lead to the introduction to a another compatible
> string.
>
> This commit split the i2c information into armada-370.dtsi and
> armada-xp.dtsi. Most of the data remains the same and stay in the
> common file Armada-370-xp.dtsi. With this new feature the size of the
> registers are bigger for Armada XP and the new compatible string is
> used.
>
> The Device Tree binding documentation is updated accordingly.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 13 ++++++++++++-
> arch/arm/boot/dts/armada-370-xp.dtsi | 2 --
> arch/arm/boot/dts/armada-370.dtsi | 8 ++++++++
> arch/arm/boot/dts/armada-xp.dtsi | 10 ++++++++++
> 4 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> index a1ee681..c5dd952 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> @@ -4,7 +4,8 @@
> Required properties :
>
> - reg : Offset and length of the register set for the device
> - - compatible : Should be "marvell,mv64xxx-i2c"
> + - compatible : Should be "marvell,mv64xxx-i2c" and "marvell,mv7230-i2c"
> +for controller which support the I2C Transaction Generator
> - interrupts : The interrupt number
As you've preserved backwards compatibility with the original compat
string,
Applied to mvebu/dt
thx,
Jason.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c
@ 2013-08-03 17:36 ` Jason Cooper
0 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2013-08-03 17:36 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 15, 2013 at 04:24:38PM +0200, Gregory CLEMENT wrote:
> The mv64xxx-i2c embedded in the Armada XP have a new feature to
> offload i2c transaction. This new version of the IP come also with
> some errata. This lead to the introduction to a another compatible
> string.
>
> This commit split the i2c information into armada-370.dtsi and
> armada-xp.dtsi. Most of the data remains the same and stay in the
> common file Armada-370-xp.dtsi. With this new feature the size of the
> registers are bigger for Armada XP and the new compatible string is
> used.
>
> The Device Tree binding documentation is updated accordingly.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
> Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 13 ++++++++++++-
> arch/arm/boot/dts/armada-370-xp.dtsi | 2 --
> arch/arm/boot/dts/armada-370.dtsi | 8 ++++++++
> arch/arm/boot/dts/armada-xp.dtsi | 10 ++++++++++
> 4 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> index a1ee681..c5dd952 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> @@ -4,7 +4,8 @@
> Required properties :
>
> - reg : Offset and length of the register set for the device
> - - compatible : Should be "marvell,mv64xxx-i2c"
> + - compatible : Should be "marvell,mv64xxx-i2c" and "marvell,mv7230-i2c"
> +for controller which support the I2C Transaction Generator
> - interrupts : The interrupt number
As you've preserved backwards compatibility with the original compat
string,
Applied to mvebu/dt
thx,
Jason.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
2013-07-16 8:05 ` Maxime Ripard
@ 2013-08-06 12:05 ` Gregory CLEMENT
-1 siblings, 0 replies; 34+ messages in thread
From: Gregory CLEMENT @ 2013-08-06 12:05 UTC (permalink / raw)
To: Maxime Ripard, Wolfram Sang
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Jason Cooper, Andrew Lunn,
Thomas Petazzoni, Lior Amsalem, Yehuda Yitschak, Ike Pan,
Piotr Ziecik, Tawfik Bayouk, Nicolas Pitre, Dan Frazier,
Chris Van Hoof, David Marlin, Eran Ben-Avi, Nadav Haklai,
Maen Suleiman, Shadi Ammouri, Ezequiel Garcia, Jon Masters,
Leif Lindholm, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Sebastian Hesselbarth
On 16/07/2013 10:05, Maxime Ripard wrote:
> Hi Gregory,
>
> On Mon, Jul 15, 2013 at 04:24:36PM +0200, Gregory CLEMENT wrote:
>> The I2C Transaction Generator offloads CPU from managing I2C transfer step by step.
>>
>> This feature is currently only available on Armada XP, so usage of this mechanism is activated through device tree.
>>
>> Based on the work of Piotr Ziecik and rewrote to use the new way of handling multiples i2c messages.
>>
>> Signed-off-by: Piotr Ziecik <kosmo-nYOzD4b6Jr9Wk0Htik3J/w@public.gmane.org> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> --- drivers/i2c/busses/i2c-mv64xxx.c | 207
>> ++++++++++++++++++++++++++++++++++++--- 1 file changed, 196 insertions(+), 11 deletions(-)
>
> [...]
>
>> + /* + * 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; +
>
> Do you have a reason for not adding it to the match table? I mean, you will introduce a new compatible here, but if that compatible is used alone, won't probe the driver? That doesn't
> seem very right to me.
But we shouldn't use it alone: we should always use:
compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>From my point of view using "marvell,mv78230-i2c" alone is an error.
Wolfram what is your opinion on it?
>
> Also, you should probably add it to the bindings documentation.
See the patch 3 for the bindings documentation.
>
> Maxime
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
@ 2013-08-06 12:05 ` Gregory CLEMENT
0 siblings, 0 replies; 34+ messages in thread
From: Gregory CLEMENT @ 2013-08-06 12:05 UTC (permalink / raw)
To: linux-arm-kernel
On 16/07/2013 10:05, Maxime Ripard wrote:
> Hi Gregory,
>
> On Mon, Jul 15, 2013 at 04:24:36PM +0200, Gregory CLEMENT wrote:
>> The I2C Transaction Generator offloads CPU from managing I2C transfer step by step.
>>
>> This feature is currently only available on Armada XP, so usage of this mechanism is activated through device tree.
>>
>> Based on the work of Piotr Ziecik and rewrote to use the new way of handling multiples i2c messages.
>>
>> Signed-off-by: Piotr Ziecik <kosmo@semihalf.com> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> --- drivers/i2c/busses/i2c-mv64xxx.c | 207
>> ++++++++++++++++++++++++++++++++++++--- 1 file changed, 196 insertions(+), 11 deletions(-)
>
> [...]
>
>> + /* + * 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; +
>
> Do you have a reason for not adding it to the match table? I mean, you will introduce a new compatible here, but if that compatible is used alone, won't probe the driver? That doesn't
> seem very right to me.
But we shouldn't use it alone: we should always use:
compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>From my point of view using "marvell,mv78230-i2c" alone is an error.
Wolfram what is your opinion on it?
>
> Also, you should probably add it to the bindings documentation.
See the patch 3 for the bindings documentation.
>
> Maxime
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
2013-08-06 12:05 ` Gregory CLEMENT
@ 2013-08-06 16:21 ` Maxime Ripard
-1 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2013-08-06 16:21 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Wolfram Sang, linux-i2c-u79uwXL29TY76Z2rM5mHXA, Jason Cooper,
Andrew Lunn, Thomas Petazzoni, Lior Amsalem, Yehuda Yitschak,
Ike Pan, Piotr Ziecik, Tawfik Bayouk, Nicolas Pitre, Dan Frazier,
Chris Van Hoof, David Marlin, Eran Ben-Avi, Nadav Haklai,
Maen Suleiman, Shadi Ammouri, Ezequiel Garcia, Jon Masters,
Leif Lindholm, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Sebastian Hesselbarth
[-- Attachment #1: Type: text/plain, Size: 1767 bytes --]
On Tue, Aug 06, 2013 at 02:05:24PM +0200, Gregory CLEMENT wrote:
> On 16/07/2013 10:05, Maxime Ripard wrote:
> > Hi Gregory,
> >
> > On Mon, Jul 15, 2013 at 04:24:36PM +0200, Gregory CLEMENT wrote:
> >> The I2C Transaction Generator offloads CPU from managing I2C transfer step by step.
> >>
> >> This feature is currently only available on Armada XP, so usage of this mechanism is activated through device tree.
> >>
> >> Based on the work of Piotr Ziecik and rewrote to use the new way of handling multiples i2c messages.
> >>
> >> Signed-off-by: Piotr Ziecik <kosmo-nYOzD4b6Jr9Wk0Htik3J/w@public.gmane.org> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> --- drivers/i2c/busses/i2c-mv64xxx.c | 207
> >> ++++++++++++++++++++++++++++++++++++--- 1 file changed, 196 insertions(+), 11 deletions(-)
> >
> > [...]
> >
> >> + /* + * 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; +
> >
> > Do you have a reason for not adding it to the match table? I mean, you will introduce a new compatible here, but if that compatible is used alone, won't probe the driver? That doesn't
> > seem very right to me.
>
> But we shouldn't use it alone: we should always use:
> compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>
> From my point of view using "marvell,mv78230-i2c" alone is an error.
Why is that? If the I2C controller is a new IP with additional features,
it should have a full compatible of its own, doesn't it?
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
@ 2013-08-06 16:21 ` Maxime Ripard
0 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2013-08-06 16:21 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 06, 2013 at 02:05:24PM +0200, Gregory CLEMENT wrote:
> On 16/07/2013 10:05, Maxime Ripard wrote:
> > Hi Gregory,
> >
> > On Mon, Jul 15, 2013 at 04:24:36PM +0200, Gregory CLEMENT wrote:
> >> The I2C Transaction Generator offloads CPU from managing I2C transfer step by step.
> >>
> >> This feature is currently only available on Armada XP, so usage of this mechanism is activated through device tree.
> >>
> >> Based on the work of Piotr Ziecik and rewrote to use the new way of handling multiples i2c messages.
> >>
> >> Signed-off-by: Piotr Ziecik <kosmo@semihalf.com> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> --- drivers/i2c/busses/i2c-mv64xxx.c | 207
> >> ++++++++++++++++++++++++++++++++++++--- 1 file changed, 196 insertions(+), 11 deletions(-)
> >
> > [...]
> >
> >> + /* + * 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; +
> >
> > Do you have a reason for not adding it to the match table? I mean, you will introduce a new compatible here, but if that compatible is used alone, won't probe the driver? That doesn't
> > seem very right to me.
>
> But we shouldn't use it alone: we should always use:
> compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>
> From my point of view using "marvell,mv78230-i2c" alone is an error.
Why is that? If the I2C controller is a new IP with additional features,
it should have a full compatible of its own, doesn't it?
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- 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/20130806/58933445/attachment-0001.sig>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
2013-08-06 12:05 ` Gregory CLEMENT
@ 2013-08-06 17:55 ` Thomas Petazzoni
-1 siblings, 0 replies; 34+ messages in thread
From: Thomas Petazzoni @ 2013-08-06 17:55 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Maxime Ripard, Wolfram Sang, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
Jason Cooper, Andrew Lunn, Lior Amsalem, Yehuda Yitschak,
Ike Pan, Piotr Ziecik, Tawfik Bayouk, Nicolas Pitre, Dan Frazier,
Chris Van Hoof, David Marlin, Eran Ben-Avi, Nadav Haklai,
Maen Suleiman, Shadi Ammouri, Ezequiel Garcia, Jon Masters,
Leif Lindholm, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Sebastian Hesselbarth
Dear Gregory CLEMENT,
On Tue, 06 Aug 2013 14:05:24 +0200, Gregory CLEMENT wrote:
> But we shouldn't use it alone: we should always use:
> compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>
> From my point of view using "marvell,mv78230-i2c" alone is an error.
I disagree. I believe the driver should support a Device Tree file that
uses "marvell,mv78230-i2c" alone.
Best regards,
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
@ 2013-08-06 17:55 ` Thomas Petazzoni
0 siblings, 0 replies; 34+ messages in thread
From: Thomas Petazzoni @ 2013-08-06 17:55 UTC (permalink / raw)
To: linux-arm-kernel
Dear Gregory CLEMENT,
On Tue, 06 Aug 2013 14:05:24 +0200, Gregory CLEMENT wrote:
> But we shouldn't use it alone: we should always use:
> compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>
> From my point of view using "marvell,mv78230-i2c" alone is an error.
I disagree. I believe the driver should support a Device Tree file that
uses "marvell,mv78230-i2c" alone.
Best regards,
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
2013-08-06 12:05 ` Gregory CLEMENT
@ 2013-08-07 14:35 ` Wolfram Sang
-1 siblings, 0 replies; 34+ messages in thread
From: Wolfram Sang @ 2013-08-07 14:35 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Maxime Ripard, linux-i2c-u79uwXL29TY76Z2rM5mHXA, Jason Cooper,
Andrew Lunn, Thomas Petazzoni, Lior Amsalem, Yehuda Yitschak,
Ike Pan, Piotr Ziecik, Tawfik Bayouk, Nicolas Pitre, Dan Frazier,
Chris Van Hoof, David Marlin, Eran Ben-Avi, Nadav Haklai,
Maen Suleiman, Shadi Ammouri, Ezequiel Garcia, Jon Masters,
Leif Lindholm, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Sebastian Hesselbarth
[-- Attachment #1: Type: text/plain, Size: 530 bytes --]
> But we shouldn't use it alone: we should always use:
> compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>
> From my point of view using "marvell,mv78230-i2c" alone is an error.
>
> Wolfram what is your opinion on it?
It is not strictly an error, but risky. If you use an older Kernel
version (or other OS) which only offers "mv64xxx" you will have no
match. Although the driver theoretically could have basic support for
all mv64xxx variants skipping all additional features of later IP
revisions.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
@ 2013-08-07 14:35 ` Wolfram Sang
0 siblings, 0 replies; 34+ messages in thread
From: Wolfram Sang @ 2013-08-07 14:35 UTC (permalink / raw)
To: linux-arm-kernel
> But we shouldn't use it alone: we should always use:
> compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>
> From my point of view using "marvell,mv78230-i2c" alone is an error.
>
> Wolfram what is your opinion on it?
It is not strictly an error, but risky. If you use an older Kernel
version (or other OS) which only offers "mv64xxx" you will have no
match. Although the driver theoretically could have basic support for
all mv64xxx variants skipping all additional features of later IP
revisions.
-------------- 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/20130807/a53878e3/attachment.sig>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
2013-08-07 14:35 ` Wolfram Sang
@ 2013-08-07 15:57 ` Jason Cooper
-1 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2013-08-07 15:57 UTC (permalink / raw)
To: Wolfram Sang
Cc: Gregory CLEMENT, Maxime Ripard, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
Andrew Lunn, Thomas Petazzoni, Lior Amsalem, Yehuda Yitschak,
Ike Pan, Piotr Ziecik, Tawfik Bayouk, Nicolas Pitre, Dan Frazier,
Chris Van Hoof, David Marlin, Eran Ben-Avi, Nadav Haklai,
Maen Suleiman, Shadi Ammouri, Ezequiel Garcia, Jon Masters,
Leif Lindholm, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Sebastian Hesselbarth
On Wed, Aug 07, 2013 at 04:35:46PM +0200, Wolfram Sang wrote:
>
> > But we shouldn't use it alone: we should always use:
> > compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> >
> > From my point of view using "marvell,mv78230-i2c" alone is an error.
> >
> > Wolfram what is your opinion on it?
>
> It is not strictly an error, but risky. If you use an older Kernel
> version (or other OS) which only offers "mv64xxx" you will have no
> match. Although the driver theoretically could have basic support for
> all mv64xxx variants skipping all additional features of later IP
> revisions.
I agree here. The driver is advertising what IP blocks it can handle,
so it makes sense to add both strings since it can handle both.
thx,
Jason.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
@ 2013-08-07 15:57 ` Jason Cooper
0 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2013-08-07 15:57 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 07, 2013 at 04:35:46PM +0200, Wolfram Sang wrote:
>
> > But we shouldn't use it alone: we should always use:
> > compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> >
> > From my point of view using "marvell,mv78230-i2c" alone is an error.
> >
> > Wolfram what is your opinion on it?
>
> It is not strictly an error, but risky. If you use an older Kernel
> version (or other OS) which only offers "mv64xxx" you will have no
> match. Although the driver theoretically could have basic support for
> all mv64xxx variants skipping all additional features of later IP
> revisions.
I agree here. The driver is advertising what IP blocks it can handle,
so it makes sense to add both strings since it can handle both.
thx,
Jason.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
2013-08-07 15:57 ` Jason Cooper
@ 2013-08-08 15:30 ` Gregory CLEMENT
-1 siblings, 0 replies; 34+ messages in thread
From: Gregory CLEMENT @ 2013-08-08 15:30 UTC (permalink / raw)
To: Wolfram Sang
Cc: Jason Cooper, Maxime Ripard, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
Andrew Lunn, Thomas Petazzoni, Lior Amsalem, Yehuda Yitschak,
Ike Pan, Piotr Ziecik, Tawfik Bayouk, Nicolas Pitre, Dan Frazier,
Chris Van Hoof, David Marlin, Eran Ben-Avi, Nadav Haklai,
Maen Suleiman, Shadi Ammouri, Ezequiel Garcia, Jon Masters,
Leif Lindholm, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Sebastian Hesselbarth
On 07/08/2013 17:57, Jason Cooper wrote:
> On Wed, Aug 07, 2013 at 04:35:46PM +0200, Wolfram Sang wrote:
>>
>>> But we shouldn't use it alone: we should always use:
>>> compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>>>
>>> From my point of view using "marvell,mv78230-i2c" alone is an error.
>>>
>>> Wolfram what is your opinion on it?
>>
>> It is not strictly an error, but risky. If you use an older Kernel
>> version (or other OS) which only offers "mv64xxx" you will have no
>> match. Although the driver theoretically could have basic support for
>> all mv64xxx variants skipping all additional features of later IP
>> revisions.
>
> I agree here. The driver is advertising what IP blocks it can handle,
> so it makes sense to add both strings since it can handle both.
Wolfram,
so beside remarks about the compatibility strings. I didn't any other
comment since the v3 which was 7 weeks ago.
Does it mean that once I will have added the handle of this string,
you will be able to take the series for the 3.12 kernel?
Thanks,
>
> thx,
>
> Jason.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
@ 2013-08-08 15:30 ` Gregory CLEMENT
0 siblings, 0 replies; 34+ messages in thread
From: Gregory CLEMENT @ 2013-08-08 15:30 UTC (permalink / raw)
To: linux-arm-kernel
On 07/08/2013 17:57, Jason Cooper wrote:
> On Wed, Aug 07, 2013 at 04:35:46PM +0200, Wolfram Sang wrote:
>>
>>> But we shouldn't use it alone: we should always use:
>>> compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>>>
>>> From my point of view using "marvell,mv78230-i2c" alone is an error.
>>>
>>> Wolfram what is your opinion on it?
>>
>> It is not strictly an error, but risky. If you use an older Kernel
>> version (or other OS) which only offers "mv64xxx" you will have no
>> match. Although the driver theoretically could have basic support for
>> all mv64xxx variants skipping all additional features of later IP
>> revisions.
>
> I agree here. The driver is advertising what IP blocks it can handle,
> so it makes sense to add both strings since it can handle both.
Wolfram,
so beside remarks about the compatibility strings. I didn't any other
comment since the v3 which was 7 weeks ago.
Does it mean that once I will have added the handle of this string,
you will be able to take the series for the 3.12 kernel?
Thanks,
>
> thx,
>
> Jason.
> --
> 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
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
2013-08-08 15:30 ` Gregory CLEMENT
@ 2013-08-08 16:43 ` Mark Rutland
-1 siblings, 0 replies; 34+ messages in thread
From: Mark Rutland @ 2013-08-08 16:43 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Lior Amsalem, Andrew Lunn, Ike Pan, Wolfram Sang, Nadav Haklai,
linux-i2c, David Marlin, Yehuda Yitschak, Tawfik Bayouk,
dann.frazier, Eran Ben-Avi, Ezequiel Garcia, Leif Lindholm,
Sebastian Hesselbarth, Jason Cooper, jcm, linux-arm-kernel,
Thomas Petazzoni, vanhoof, Piotr Ziecik, Nicolas
On Thu, Aug 08, 2013 at 04:30:02PM +0100, Gregory CLEMENT wrote:
> On 07/08/2013 17:57, Jason Cooper wrote:
> > On Wed, Aug 07, 2013 at 04:35:46PM +0200, Wolfram Sang wrote:
> >>
> >>> But we shouldn't use it alone: we should always use:
> >>> compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> >>>
> >>> From my point of view using "marvell,mv78230-i2c" alone is an error.
> >>>
> >>> Wolfram what is your opinion on it?
> >>
> >> It is not strictly an error, but risky. If you use an older Kernel
> >> version (or other OS) which only offers "mv64xxx" you will have no
> >> match. Although the driver theoretically could have basic support for
> >> all mv64xxx variants skipping all additional features of later IP
> >> revisions.
> >
> > I agree here. The driver is advertising what IP blocks it can handle,
> > so it makes sense to add both strings since it can handle both.
>
> Wolfram,
> so beside remarks about the compatibility strings. I didn't any other
> comment since the v3 which was 7 weeks ago.
>
> Does it mean that once I will have added the handle of this string,
> you will be able to take the series for the 3.12 kernel?
Please could you also ensure the new string is documented in
Documentation/devicetree, with a brief description of what it implies
about the hardware beyond the exiting "marvell,mv64xxx-i2c" string.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
@ 2013-08-08 16:43 ` Mark Rutland
0 siblings, 0 replies; 34+ messages in thread
From: Mark Rutland @ 2013-08-08 16:43 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Aug 08, 2013 at 04:30:02PM +0100, Gregory CLEMENT wrote:
> On 07/08/2013 17:57, Jason Cooper wrote:
> > On Wed, Aug 07, 2013 at 04:35:46PM +0200, Wolfram Sang wrote:
> >>
> >>> But we shouldn't use it alone: we should always use:
> >>> compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> >>>
> >>> From my point of view using "marvell,mv78230-i2c" alone is an error.
> >>>
> >>> Wolfram what is your opinion on it?
> >>
> >> It is not strictly an error, but risky. If you use an older Kernel
> >> version (or other OS) which only offers "mv64xxx" you will have no
> >> match. Although the driver theoretically could have basic support for
> >> all mv64xxx variants skipping all additional features of later IP
> >> revisions.
> >
> > I agree here. The driver is advertising what IP blocks it can handle,
> > so it makes sense to add both strings since it can handle both.
>
> Wolfram,
> so beside remarks about the compatibility strings. I didn't any other
> comment since the v3 which was 7 weeks ago.
>
> Does it mean that once I will have added the handle of this string,
> you will be able to take the series for the 3.12 kernel?
Please could you also ensure the new string is documented in
Documentation/devicetree, with a brief description of what it implies
about the hardware beyond the exiting "marvell,mv64xxx-i2c" string.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
2013-08-08 16:43 ` Mark Rutland
@ 2013-08-08 17:02 ` Jason Cooper
-1 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2013-08-08 17:02 UTC (permalink / raw)
To: Mark Rutland
Cc: Lior Amsalem, Andrew Lunn, Ike Pan, Wolfram Sang, Nadav Haklai,
linux-i2c, David Marlin, Yehuda Yitschak, Tawfik Bayouk,
dann.frazier, Eran Ben-Avi, Ezequiel Garcia, Leif Lindholm,
Sebastian Hesselbarth, jcm, Gregory CLEMENT, linux-arm-kernel,
Thomas Petazzoni, vanhoof, Piotr Ziecik
On Thu, Aug 08, 2013 at 05:43:23PM +0100, Mark Rutland wrote:
> On Thu, Aug 08, 2013 at 04:30:02PM +0100, Gregory CLEMENT wrote:
> > On 07/08/2013 17:57, Jason Cooper wrote:
> > > On Wed, Aug 07, 2013 at 04:35:46PM +0200, Wolfram Sang wrote:
> > >>
> > >>> But we shouldn't use it alone: we should always use:
> > >>> compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> > >>>
> > >>> From my point of view using "marvell,mv78230-i2c" alone is an error.
> > >>>
> > >>> Wolfram what is your opinion on it?
> > >>
> > >> It is not strictly an error, but risky. If you use an older Kernel
> > >> version (or other OS) which only offers "mv64xxx" you will have no
> > >> match. Although the driver theoretically could have basic support for
> > >> all mv64xxx variants skipping all additional features of later IP
> > >> revisions.
> > >
> > > I agree here. The driver is advertising what IP blocks it can handle,
> > > so it makes sense to add both strings since it can handle both.
> >
> > Wolfram,
> > so beside remarks about the compatibility strings. I didn't any other
> > comment since the v3 which was 7 weeks ago.
> >
> > Does it mean that once I will have added the handle of this string,
> > you will be able to take the series for the 3.12 kernel?
>
> Please could you also ensure the new string is documented in
> Documentation/devicetree, with a brief description of what it implies
> about the hardware beyond the exiting "marvell,mv64xxx-i2c" string.
That was in patch 3/3:
ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c
Which I've applied here:
http://git.infradead.org/linux-mvebu.git/commitdiff/e7c4a1e9f937453a32a5119868cac49de098640a
And my reasoning for taking it:
http://marc.info/?l=linux-arm-kernel&m=137555145730141&w=2
I haven't sent a PR to arm-soc yet (I like to let things gel in -next
for a few days), so if there's something wrong with it, please let me
know.
thx,
Jason.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
@ 2013-08-08 17:02 ` Jason Cooper
0 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2013-08-08 17:02 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Aug 08, 2013 at 05:43:23PM +0100, Mark Rutland wrote:
> On Thu, Aug 08, 2013 at 04:30:02PM +0100, Gregory CLEMENT wrote:
> > On 07/08/2013 17:57, Jason Cooper wrote:
> > > On Wed, Aug 07, 2013 at 04:35:46PM +0200, Wolfram Sang wrote:
> > >>
> > >>> But we shouldn't use it alone: we should always use:
> > >>> compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> > >>>
> > >>> From my point of view using "marvell,mv78230-i2c" alone is an error.
> > >>>
> > >>> Wolfram what is your opinion on it?
> > >>
> > >> It is not strictly an error, but risky. If you use an older Kernel
> > >> version (or other OS) which only offers "mv64xxx" you will have no
> > >> match. Although the driver theoretically could have basic support for
> > >> all mv64xxx variants skipping all additional features of later IP
> > >> revisions.
> > >
> > > I agree here. The driver is advertising what IP blocks it can handle,
> > > so it makes sense to add both strings since it can handle both.
> >
> > Wolfram,
> > so beside remarks about the compatibility strings. I didn't any other
> > comment since the v3 which was 7 weeks ago.
> >
> > Does it mean that once I will have added the handle of this string,
> > you will be able to take the series for the 3.12 kernel?
>
> Please could you also ensure the new string is documented in
> Documentation/devicetree, with a brief description of what it implies
> about the hardware beyond the exiting "marvell,mv64xxx-i2c" string.
That was in patch 3/3:
ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c
Which I've applied here:
http://git.infradead.org/linux-mvebu.git/commitdiff/e7c4a1e9f937453a32a5119868cac49de098640a
And my reasoning for taking it:
http://marc.info/?l=linux-arm-kernel&m=137555145730141&w=2
I haven't sent a PR to arm-soc yet (I like to let things gel in -next
for a few days), so if there's something wrong with it, please let me
know.
thx,
Jason.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
2013-08-08 17:02 ` Jason Cooper
@ 2013-08-08 17:09 ` Mark Rutland
-1 siblings, 0 replies; 34+ messages in thread
From: Mark Rutland @ 2013-08-08 17:09 UTC (permalink / raw)
To: Jason Cooper
Cc: Lior Amsalem, Andrew Lunn, Ike Pan, Wolfram Sang, Nadav Haklai,
linux-i2c, David Marlin, Yehuda Yitschak, Tawfik Bayouk,
dann.frazier, Eran Ben-Avi, Ezequiel Garcia, Leif Lindholm,
Sebastian Hesselbarth, jcm, Gregory CLEMENT, linux-arm-kernel,
Thomas Petazzoni, vanhoof, Piotr Ziecik
On Thu, Aug 08, 2013 at 06:02:38PM +0100, Jason Cooper wrote:
> On Thu, Aug 08, 2013 at 05:43:23PM +0100, Mark Rutland wrote:
> > On Thu, Aug 08, 2013 at 04:30:02PM +0100, Gregory CLEMENT wrote:
> > > On 07/08/2013 17:57, Jason Cooper wrote:
> > > > On Wed, Aug 07, 2013 at 04:35:46PM +0200, Wolfram Sang wrote:
> > > >>
> > > >>> But we shouldn't use it alone: we should always use:
> > > >>> compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> > > >>>
> > > >>> From my point of view using "marvell,mv78230-i2c" alone is an error.
> > > >>>
> > > >>> Wolfram what is your opinion on it?
> > > >>
> > > >> It is not strictly an error, but risky. If you use an older Kernel
> > > >> version (or other OS) which only offers "mv64xxx" you will have no
> > > >> match. Although the driver theoretically could have basic support for
> > > >> all mv64xxx variants skipping all additional features of later IP
> > > >> revisions.
> > > >
> > > > I agree here. The driver is advertising what IP blocks it can handle,
> > > > so it makes sense to add both strings since it can handle both.
> > >
> > > Wolfram,
> > > so beside remarks about the compatibility strings. I didn't any other
> > > comment since the v3 which was 7 weeks ago.
> > >
> > > Does it mean that once I will have added the handle of this string,
> > > you will be able to take the series for the 3.12 kernel?
> >
> > Please could you also ensure the new string is documented in
> > Documentation/devicetree, with a brief description of what it implies
> > about the hardware beyond the exiting "marvell,mv64xxx-i2c" string.
>
> That was in patch 3/3:
>
> ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c
>
> Which I've applied here:
>
> http://git.infradead.org/linux-mvebu.git/commitdiff/e7c4a1e9f937453a32a5119868cac49de098640a
>
> And my reasoning for taking it:
>
> http://marc.info/?l=linux-arm-kernel&m=137555145730141&w=2
My bad, I missed that.
>
> I haven't sent a PR to arm-soc yet (I like to let things gel in -next
> for a few days), so if there's something wrong with it, please let me
> know.
It looks fine to me. :)
Thanks,
Mark.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
@ 2013-08-08 17:09 ` Mark Rutland
0 siblings, 0 replies; 34+ messages in thread
From: Mark Rutland @ 2013-08-08 17:09 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Aug 08, 2013 at 06:02:38PM +0100, Jason Cooper wrote:
> On Thu, Aug 08, 2013 at 05:43:23PM +0100, Mark Rutland wrote:
> > On Thu, Aug 08, 2013 at 04:30:02PM +0100, Gregory CLEMENT wrote:
> > > On 07/08/2013 17:57, Jason Cooper wrote:
> > > > On Wed, Aug 07, 2013 at 04:35:46PM +0200, Wolfram Sang wrote:
> > > >>
> > > >>> But we shouldn't use it alone: we should always use:
> > > >>> compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> > > >>>
> > > >>> From my point of view using "marvell,mv78230-i2c" alone is an error.
> > > >>>
> > > >>> Wolfram what is your opinion on it?
> > > >>
> > > >> It is not strictly an error, but risky. If you use an older Kernel
> > > >> version (or other OS) which only offers "mv64xxx" you will have no
> > > >> match. Although the driver theoretically could have basic support for
> > > >> all mv64xxx variants skipping all additional features of later IP
> > > >> revisions.
> > > >
> > > > I agree here. The driver is advertising what IP blocks it can handle,
> > > > so it makes sense to add both strings since it can handle both.
> > >
> > > Wolfram,
> > > so beside remarks about the compatibility strings. I didn't any other
> > > comment since the v3 which was 7 weeks ago.
> > >
> > > Does it mean that once I will have added the handle of this string,
> > > you will be able to take the series for the 3.12 kernel?
> >
> > Please could you also ensure the new string is documented in
> > Documentation/devicetree, with a brief description of what it implies
> > about the hardware beyond the exiting "marvell,mv64xxx-i2c" string.
>
> That was in patch 3/3:
>
> ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c
>
> Which I've applied here:
>
> http://git.infradead.org/linux-mvebu.git/commitdiff/e7c4a1e9f937453a32a5119868cac49de098640a
>
> And my reasoning for taking it:
>
> http://marc.info/?l=linux-arm-kernel&m=137555145730141&w=2
My bad, I missed that.
>
> I haven't sent a PR to arm-soc yet (I like to let things gel in -next
> for a few days), so if there's something wrong with it, please let me
> know.
It looks fine to me. :)
Thanks,
Mark.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
2013-08-08 17:09 ` Mark Rutland
@ 2013-08-08 17:17 ` Jason Cooper
-1 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2013-08-08 17:17 UTC (permalink / raw)
To: Mark Rutland
Cc: Lior Amsalem, Andrew Lunn, Ike Pan, Wolfram Sang, Nadav Haklai,
linux-i2c, David Marlin, Yehuda Yitschak, Tawfik Bayouk,
dann.frazier, Eran Ben-Avi, Ezequiel Garcia, Leif Lindholm,
Sebastian Hesselbarth, jcm, Gregory CLEMENT, linux-arm-kernel,
Thomas Petazzoni, vanhoof, Piotr Ziecik
On Thu, Aug 08, 2013 at 06:09:01PM +0100, Mark Rutland wrote:
> On Thu, Aug 08, 2013 at 06:02:38PM +0100, Jason Cooper wrote:
> > On Thu, Aug 08, 2013 at 05:43:23PM +0100, Mark Rutland wrote:
> > > On Thu, Aug 08, 2013 at 04:30:02PM +0100, Gregory CLEMENT wrote:
> > > > On 07/08/2013 17:57, Jason Cooper wrote:
> > > > > On Wed, Aug 07, 2013 at 04:35:46PM +0200, Wolfram Sang wrote:
> > > > >>
> > > > >>> But we shouldn't use it alone: we should always use:
> > > > >>> compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> > > > >>>
> > > > >>> From my point of view using "marvell,mv78230-i2c" alone is an error.
> > > > >>>
> > > > >>> Wolfram what is your opinion on it?
> > > > >>
> > > > >> It is not strictly an error, but risky. If you use an older Kernel
> > > > >> version (or other OS) which only offers "mv64xxx" you will have no
> > > > >> match. Although the driver theoretically could have basic support for
> > > > >> all mv64xxx variants skipping all additional features of later IP
> > > > >> revisions.
> > > > >
> > > > > I agree here. The driver is advertising what IP blocks it can handle,
> > > > > so it makes sense to add both strings since it can handle both.
> > > >
> > > > Wolfram,
> > > > so beside remarks about the compatibility strings. I didn't any other
> > > > comment since the v3 which was 7 weeks ago.
> > > >
> > > > Does it mean that once I will have added the handle of this string,
> > > > you will be able to take the series for the 3.12 kernel?
> > >
> > > Please could you also ensure the new string is documented in
> > > Documentation/devicetree, with a brief description of what it implies
> > > about the hardware beyond the exiting "marvell,mv64xxx-i2c" string.
> >
> > That was in patch 3/3:
> >
> > ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c
> >
> > Which I've applied here:
> >
> > http://git.infradead.org/linux-mvebu.git/commitdiff/e7c4a1e9f937453a32a5119868cac49de098640a
> >
> > And my reasoning for taking it:
> >
> > http://marc.info/?l=linux-arm-kernel&m=137555145730141&w=2
>
> My bad, I missed that.
No problem, I know you guys are drinking from a fire hose. :)
> > I haven't sent a PR to arm-soc yet (I like to let things gel in -next
> > for a few days), so if there's something wrong with it, please let me
> > know.
>
> It looks fine to me. :)
Great!
thx,
Jason.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
@ 2013-08-08 17:17 ` Jason Cooper
0 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2013-08-08 17:17 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Aug 08, 2013 at 06:09:01PM +0100, Mark Rutland wrote:
> On Thu, Aug 08, 2013 at 06:02:38PM +0100, Jason Cooper wrote:
> > On Thu, Aug 08, 2013 at 05:43:23PM +0100, Mark Rutland wrote:
> > > On Thu, Aug 08, 2013 at 04:30:02PM +0100, Gregory CLEMENT wrote:
> > > > On 07/08/2013 17:57, Jason Cooper wrote:
> > > > > On Wed, Aug 07, 2013 at 04:35:46PM +0200, Wolfram Sang wrote:
> > > > >>
> > > > >>> But we shouldn't use it alone: we should always use:
> > > > >>> compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
> > > > >>>
> > > > >>> From my point of view using "marvell,mv78230-i2c" alone is an error.
> > > > >>>
> > > > >>> Wolfram what is your opinion on it?
> > > > >>
> > > > >> It is not strictly an error, but risky. If you use an older Kernel
> > > > >> version (or other OS) which only offers "mv64xxx" you will have no
> > > > >> match. Although the driver theoretically could have basic support for
> > > > >> all mv64xxx variants skipping all additional features of later IP
> > > > >> revisions.
> > > > >
> > > > > I agree here. The driver is advertising what IP blocks it can handle,
> > > > > so it makes sense to add both strings since it can handle both.
> > > >
> > > > Wolfram,
> > > > so beside remarks about the compatibility strings. I didn't any other
> > > > comment since the v3 which was 7 weeks ago.
> > > >
> > > > Does it mean that once I will have added the handle of this string,
> > > > you will be able to take the series for the 3.12 kernel?
> > >
> > > Please could you also ensure the new string is documented in
> > > Documentation/devicetree, with a brief description of what it implies
> > > about the hardware beyond the exiting "marvell,mv64xxx-i2c" string.
> >
> > That was in patch 3/3:
> >
> > ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c
> >
> > Which I've applied here:
> >
> > http://git.infradead.org/linux-mvebu.git/commitdiff/e7c4a1e9f937453a32a5119868cac49de098640a
> >
> > And my reasoning for taking it:
> >
> > http://marc.info/?l=linux-arm-kernel&m=137555145730141&w=2
>
> My bad, I missed that.
No problem, I know you guys are drinking from a fire hose. :)
> > I haven't sent a PR to arm-soc yet (I like to let things gel in -next
> > for a few days), so if there's something wrong with it, please let me
> > know.
>
> It looks fine to me. :)
Great!
thx,
Jason.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
2013-08-08 15:30 ` Gregory CLEMENT
@ 2013-08-09 13:22 ` Wolfram Sang
-1 siblings, 0 replies; 34+ messages in thread
From: Wolfram Sang @ 2013-08-09 13:22 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Jason Cooper, Maxime Ripard, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
Andrew Lunn, Thomas Petazzoni, Lior Amsalem, Yehuda Yitschak,
Ike Pan, Piotr Ziecik, Tawfik Bayouk, Nicolas Pitre, Dan Frazier,
Chris Van Hoof, David Marlin, Eran Ben-Avi, Nadav Haklai,
Maen Suleiman, Shadi Ammouri, Ezequiel Garcia, Jon Masters,
Leif Lindholm, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Sebastian Hesselbarth
> Does it mean that once I will have added the handle of this string,
> you will be able to take the series for the 3.12 kernel?
Most looks good. The only thing I noticed from a glimpse is that maybe
you could use be32_to_cpu or something instead of shifting 8 bit chunks
out of the data_hi/lo?
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
@ 2013-08-09 13:22 ` Wolfram Sang
0 siblings, 0 replies; 34+ messages in thread
From: Wolfram Sang @ 2013-08-09 13:22 UTC (permalink / raw)
To: linux-arm-kernel
> Does it mean that once I will have added the handle of this string,
> you will be able to take the series for the 3.12 kernel?
Most looks good. The only thing I noticed from a glimpse is that maybe
you could use be32_to_cpu or something instead of shifting 8 bit chunks
out of the data_hi/lo?
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2013-08-09 13:22 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-15 14:24 [PATCH 0/3] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP Gregory CLEMENT
2013-07-15 14:24 ` Gregory CLEMENT
[not found] ` <1373898278-4805-1-git-send-email-gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
2013-07-15 14:24 ` [PATCH 1/3] i2c-mv64xxx: Add I2C Transaction Generator support Gregory CLEMENT
2013-07-15 14:24 ` Gregory CLEMENT
[not found] ` <1373898278-4805-2-git-send-email-gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
2013-07-16 8:05 ` Maxime Ripard
2013-07-16 8:05 ` Maxime Ripard
2013-08-06 12:05 ` Gregory CLEMENT
2013-08-06 12:05 ` Gregory CLEMENT
[not found] ` <5200E684.5080003-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
2013-08-06 16:21 ` Maxime Ripard
2013-08-06 16:21 ` Maxime Ripard
2013-08-06 17:55 ` Thomas Petazzoni
2013-08-06 17:55 ` Thomas Petazzoni
2013-08-07 14:35 ` Wolfram Sang
2013-08-07 14:35 ` Wolfram Sang
2013-08-07 15:57 ` Jason Cooper
2013-08-07 15:57 ` Jason Cooper
[not found] ` <20130807155700.GE19280-u4khhh1J0LxI1Ri9qeTfzeTW4wlIGRCZ@public.gmane.org>
2013-08-08 15:30 ` Gregory CLEMENT
2013-08-08 15:30 ` Gregory CLEMENT
2013-08-08 16:43 ` Mark Rutland
2013-08-08 16:43 ` Mark Rutland
2013-08-08 17:02 ` Jason Cooper
2013-08-08 17:02 ` Jason Cooper
2013-08-08 17:09 ` Mark Rutland
2013-08-08 17:09 ` Mark Rutland
2013-08-08 17:17 ` Jason Cooper
2013-08-08 17:17 ` Jason Cooper
[not found] ` <5203B97A.8060605-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
2013-08-09 13:22 ` Wolfram Sang
2013-08-09 13:22 ` Wolfram Sang
2013-07-15 14:24 ` [PATCH 2/3] i2c-mv64xxx: Fix timing issue on Armada XP (errata FE-8471889) Gregory CLEMENT
2013-07-15 14:24 ` Gregory CLEMENT
2013-07-15 14:24 ` [PATCH 3/3] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c Gregory CLEMENT
2013-07-15 14:24 ` Gregory CLEMENT
[not found] ` <1373898278-4805-4-git-send-email-gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
2013-08-03 17:36 ` Jason Cooper
2013-08-03 17:36 ` Jason Cooper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.