Hello,
this patch set adds support for the I2C Transaction Generator which
offloads CPU from managing I2C transfer step by step. This feature is
currently only available on the I2C controller IP embedded in the
Armada XP SoC. That's why usage of this mechanism is optional and
activated through device tree.
The first patch modifies the driver itself and should go through i2c
subsystem.
Due to the recent changes in i2c-mv64xxx.c file caused by Russell
King's fixes, I based this patch on the for-next branch of the i2c git
repository.
The second patch updates the device tree to be able to use this new
feature and should go through mvebu subsystem.
The 2 patches are independents for building or even at runtime, but of
course we need both of them to be able to use the I2C Transaction
Generator on the Armada XP SoC.
Thanks
Gregory CLEMENT (1):
ARM: dts: mvebu: Add the i2c-bridge capability to the mv64xxx-i2c
Zbigniew Bodek (1):
i2c-mv64xxx: Add I2C Transaction Generator support
.../devicetree/bindings/i2c/i2c-mv64xxx.txt | 6 +
arch/arm/boot/dts/armada-370-xp.dtsi | 2 -
arch/arm/boot/dts/armada-370.dtsi | 8 ++
arch/arm/boot/dts/armada-xp.dtsi | 10 ++
drivers/i2c/busses/i2c-mv64xxx.c | 143 ++++++++++++++++++++-
5 files changed, 163 insertions(+), 6 deletions(-)
--
1.8.1.2
The mv64xxx-i2c embedded in the Armada XP have a new feature called
i2c-bridge. This commit split the i2c information into armada-370.dtsi
and armada-xp.dtsi. Most of the data remains the same and stay in the
common file Armada-370-xp.dtsi. With this new feature the size of the
registers are bigger for Armada XP and for this SoCs we add a new flag
for the i2c-bridge capability.
The Device Tree binding documentation is updated accordingly.
Signed-off-by: Gregory CLEMENT <[email protected]>
---
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 6 ++++++
arch/arm/boot/dts/armada-370-xp.dtsi | 2 --
arch/arm/boot/dts/armada-370.dtsi | 8 ++++++++
arch/arm/boot/dts/armada-xp.dtsi | 10 ++++++++++
4 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
index f46d928..8ede3e7 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
@@ -8,6 +8,12 @@ Required properties :
- interrupts : The interrupt number
- clock-frequency : Desired I2C bus clock frequency in Hz.
+Optional properties :
+
+- i2c,i2c-bridge : This flag indicate that the i2c controller have the
+ Transaction Generator support and we want to use it. Not all the
+ mv64xxx controller have this feature.
+
Examples:
i2c@11000 {
diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 550eb77..b6f475c 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -112,7 +112,6 @@
i2c0: i2c@11000 {
compatible = "marvell,mv64xxx-i2c";
- reg = <0x11000 0x20>;
#address-cells = <1>;
#size-cells = <0>;
interrupts = <31>;
@@ -123,7 +122,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 aee2b18..39b26d6 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -155,6 +155,14 @@
};
};
+ i2c0: i2c@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 5b902f9..db36bfb 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -134,6 +134,16 @@
};
};
+ i2c0: i2c@11000 {
+ reg = <0x11000 0x100>;
+ i2c,i2c-bridge;
+ };
+
+ i2c1: i2c@11100 {
+ reg = <0x11100 0x100>;
+ i2c,i2c-bridge;
+ };
+
usb@50000 {
clocks = <&gateclk 18>;
};
--
1.8.1.2
From: Zbigniew Bodek <[email protected]>
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.
[[email protected]: Rewrite some part to be applied
on the the code modified by Russell King's fixes]
[[email protected]: Merge and split the commits to
push them to the accurate maintainer i2c and of]]
[[email protected]: Reword the commit log]
Signed-off-by: Piotr Ziecik <[email protected]>
Signed-off-by: Gregory CLEMENT <[email protected]>
---
drivers/i2c/busses/i2c-mv64xxx.c | 143 +++++++++++++++++++++++++++++++++++++--
1 file changed, 139 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 6356439..5376dc3 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -24,7 +24,7 @@
#include <linux/clk.h>
#include <linux/err.h>
-/* Register defines */
+/* Register defines (I2C port) */
#define MV64XXX_I2C_REG_SLAVE_ADDR 0x00
#define MV64XXX_I2C_REG_DATA 0x04
#define MV64XXX_I2C_REG_CONTROL 0x08
@@ -59,6 +59,29 @@
#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK 0xe8
#define MV64XXX_I2C_STATUS_NO_STATUS 0xf8
+/* Register defines (I2C bridge) */
+#define MV64XXX_I2C_REG_TX_DATA_LO 0xC0
+#define MV64XXX_I2C_REG_TX_DATA_HI 0xC4
+#define MV64XXX_I2C_REG_RX_DATA_LO 0xC8
+#define MV64XXX_I2C_REG_RX_DATA_HI 0xCC
+#define MV64XXX_I2C_REG_BRIDGE_CONTROL 0xD0
+#define MV64XXX_I2C_REG_BRIDGE_STATUS 0xD4
+#define MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE 0xD8
+#define MV64XXX_I2C_REG_BRIDGE_INTR_MASK 0xDC
+#define MV64XXX_I2C_REG_BRIDGE_TIMING 0xE0
+
+/* Bridge Control values */
+#define MV64XXX_I2C_BRIDGE_CONTROL_WR 0x00000001
+#define MV64XXX_I2C_BRIDGE_CONTROL_RD 0x00000002
+#define MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT 2
+#define MV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT 0x00001000
+#define MV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT 13
+#define MV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT 16
+#define MV64XXX_I2C_BRIDGE_CONTROL_ENABLE 0x00080000
+
+/* Bridge Status values */
+#define MV64XXX_I2C_BRIDGE_STATUS_ERROR 0x00000001
+
/* Driver states */
enum {
MV64XXX_I2C_STATE_INVALID,
@@ -110,6 +133,7 @@ struct mv64xxx_i2c_data {
spinlock_t lock;
struct i2c_msg *msg;
struct i2c_adapter adapter;
+ int bridge_enabled;
};
static void
@@ -157,6 +181,16 @@ mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
writel(0, drv_data->reg_base + MV64XXX_I2C_REG_EXT_SLAVE_ADDR);
writel(MV64XXX_I2C_REG_CONTROL_TWSIEN | MV64XXX_I2C_REG_CONTROL_STOP,
drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
+
+ if (drv_data->bridge_enabled) {
+ writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
+ writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_TIMING);
+ writel(0, drv_data->reg_base +
+ MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
+ writel(0, drv_data->reg_base +
+ MV64XXX_I2C_REG_BRIDGE_INTR_MASK);
+ }
+
drv_data->state = MV64XXX_I2C_STATE_IDLE;
}
@@ -368,6 +402,19 @@ mv64xxx_i2c_intr(int irq, void *dev_id)
irqreturn_t rc = IRQ_NONE;
spin_lock_irqsave(&drv_data->lock, flags);
+
+ if (drv_data->bridge_enabled) {
+ if (readl(drv_data->reg_base +
+ MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE)) {
+ writel(0, drv_data->reg_base +
+ MV64XXX_I2C_REG_BRIDGE_CONTROL);
+ writel(0, drv_data->reg_base +
+ MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
+ drv_data->block = 0;
+ wake_up(&drv_data->waitq);
+ rc = IRQ_HANDLED;
+ }
+ }
while (readl(drv_data->reg_base + MV64XXX_I2C_REG_CONTROL) &
MV64XXX_I2C_REG_CONTROL_IFLG) {
status = readl(drv_data->reg_base + MV64XXX_I2C_REG_STATUS);
@@ -446,6 +493,81 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
return drv_data->rc;
}
+static int
+mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg)
+{
+ unsigned long data_reg_hi = 0;
+ unsigned long data_reg_lo = 0;
+ unsigned long status_reg;
+ unsigned long ctrl_reg;
+ unsigned long flags;
+ unsigned int i;
+
+ /* Only regular transactions can be offloaded */
+ if ((msg->flags & ~(I2C_M_TEN | I2C_M_RD)) != 0)
+ return 1;
+
+ /* Only 1-8 byte transfers can be offloaded */
+ if (msg->len < 1 || msg->len > 8)
+ return 1;
+
+ /* Build transaction */
+ ctrl_reg = MV64XXX_I2C_BRIDGE_CONTROL_ENABLE |
+ (msg->addr << MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT);
+
+ if ((msg->flags & I2C_M_TEN) != 0)
+ ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT;
+
+ if ((msg->flags & I2C_M_RD) == 0) {
+ for (i = 0; i < 4 && i < msg->len; i++)
+ data_reg_lo = data_reg_lo |
+ (msg->buf[i] << ((i & 0x3) * 8));
+
+ for (i = 4; i < 8 && i < msg->len; i++)
+ data_reg_hi = data_reg_hi |
+ (msg->buf[i] << ((i & 0x3) * 8));
+
+ ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_WR |
+ (msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT;
+ } else {
+ ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_RD |
+ (msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT;
+ }
+
+ /* Execute transaction */
+ spin_lock_irqsave(&drv_data->lock, flags);
+ drv_data->block = 1;
+ writel(data_reg_lo, drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_LO);
+ writel(data_reg_hi, drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_HI);
+ writel(ctrl_reg, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
+ spin_unlock_irqrestore(&drv_data->lock, flags);
+
+ mv64xxx_i2c_wait_for_completion(drv_data);
+
+ spin_lock_irqsave(&drv_data->lock, flags);
+ status_reg = readl(drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_STATUS);
+ data_reg_lo = readl(drv_data->reg_base + MV64XXX_I2C_REG_RX_DATA_LO);
+ data_reg_hi = readl(drv_data->reg_base + MV64XXX_I2C_REG_RX_DATA_HI);
+ spin_unlock_irqrestore(&drv_data->lock, flags);
+
+ if (status_reg & MV64XXX_I2C_BRIDGE_STATUS_ERROR)
+ return -EIO;
+
+ if ((msg->flags & I2C_M_RD) != 0) {
+ for (i = 0; i < 4 && i < msg->len; i++) {
+ msg->buf[i] = data_reg_lo & 0xFF;
+ data_reg_lo >>= 8;
+ }
+
+ for (i = 4; i < 8 && i < msg->len; i++) {
+ msg->buf[i] = data_reg_hi & 0xFF;
+ data_reg_hi >>= 8;
+ }
+ }
+
+ return 0;
+}
+
/*
*****************************************************************************
*
@@ -463,13 +585,15 @@ static int
mv64xxx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
{
struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
- int rc, ret = num;
+ int rc = 1, ret = num;
BUG_ON(drv_data->msgs != NULL);
drv_data->msgs = msgs;
drv_data->num_msgs = num;
-
- rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[0], num == 1);
+ if (drv_data->bridge_enabled)
+ rc = mv64xxx_i2c_offload_msg(drv_data, &msgs[0]);
+ if (rc > 0)
+ rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[0], num == 1);
if (rc < 0)
ret = rc;
@@ -528,6 +652,7 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
{
u32 bus_freq, tclk;
int rc = 0;
+ const char *bridge_status;
/* CLK is mandatory when using DT to describe the i2c bus. We
* need to know tclk in order to calculate bus clock
@@ -554,6 +679,15 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
* So hard code the value to 1 second.
*/
drv_data->adapter.timeout = HZ;
+
+ /*
+ * Acquire information on Transaction Generator support.
+ */
+ bridge_status = of_get_property(np, "", NULL);
+ if (of_property_read_bool(np, "i2c,i2c-bridge"))
+ drv_data->bridge_enabled = 1;
+ else
+ drv_data->bridge_enabled = 0;
out:
return rc;
#endif
@@ -607,6 +741,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
drv_data->freq_n = pdata->freq_n;
drv_data->irq = platform_get_irq(pd, 0);
drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
+ drv_data->bridge_enabled = 0;
} else if (pd->dev.of_node) {
rc = mv64xxx_of_config(drv_data, pd->dev.of_node);
if (rc)
--
1.8.1.2
Dear Gregory CLEMENT,
On Fri, 7 Jun 2013 17:42:22 +0200, Gregory CLEMENT wrote:
> /* Driver states */
> enum {
> MV64XXX_I2C_STATE_INVALID,
> @@ -110,6 +133,7 @@ struct mv64xxx_i2c_data {
> spinlock_t lock;
> struct i2c_msg *msg;
> struct i2c_adapter adapter;
> + int bridge_enabled;
bool ?
> @@ -528,6 +652,7 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
> {
> u32 bus_freq, tclk;
> int rc = 0;
> + const char *bridge_status;
>
> /* CLK is mandatory when using DT to describe the i2c bus. We
> * need to know tclk in order to calculate bus clock
> @@ -554,6 +679,15 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
> * So hard code the value to 1 second.
> */
> drv_data->adapter.timeout = HZ;
> +
> + /*
> + * Acquire information on Transaction Generator support.
> + */
> + bridge_status = of_get_property(np, "", NULL);
It is not clear to me what this new bridge_status local variable is
doing.
> + if (of_property_read_bool(np, "i2c,i2c-bridge"))
> + drv_data->bridge_enabled = 1;
> + else
> + drv_data->bridge_enabled = 0;
This could be written directly as:
drv_data->bridge_enabled = of_property_read_bool(np, "i2c,i2c-bridge");
Thanks,
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Hi Greg,
On Fri, Jun 07, 2013 at 05:42:23PM +0200, Gregory CLEMENT wrote:
> The mv64xxx-i2c embedded in the Armada XP have a new feature called
> i2c-bridge. This commit split the i2c information into armada-370.dtsi
> and armada-xp.dtsi. Most of the data remains the same and stay in the
> common file Armada-370-xp.dtsi. With this new feature the size of the
> registers are bigger for Armada XP and for this SoCs we add a new flag
> for the i2c-bridge capability.
>
> The Device Tree binding documentation is updated accordingly.
>
> Signed-off-by: Gregory CLEMENT <[email protected]>
> ---
> Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 6 ++++++
> arch/arm/boot/dts/armada-370-xp.dtsi | 2 --
> arch/arm/boot/dts/armada-370.dtsi | 8 ++++++++
> arch/arm/boot/dts/armada-xp.dtsi | 10 ++++++++++
> 4 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> index f46d928..8ede3e7 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> @@ -8,6 +8,12 @@ Required properties :
> - interrupts : The interrupt number
> - clock-frequency : Desired I2C bus clock frequency in Hz.
>
> +Optional properties :
> +
> +- i2c,i2c-bridge : This flag indicate that the i2c controller have the
> + Transaction Generator support and we want to use it. Not all the
> + mv64xxx controller have this feature.
Why not using a different compatible string here then?
Maxime
On Fri, Jun 07, 2013 at 05:42:23PM +0200, Gregory CLEMENT wrote:
> The mv64xxx-i2c embedded in the Armada XP have a new feature called
> i2c-bridge. This commit split the i2c information into armada-370.dtsi
> and armada-xp.dtsi. Most of the data remains the same and stay in the
> common file Armada-370-xp.dtsi. With this new feature the size of the
> registers are bigger for Armada XP and for this SoCs we add a new flag
> for the i2c-bridge capability.
>
> The Device Tree binding documentation is updated accordingly.
>
> Signed-off-by: Gregory CLEMENT <[email protected]>
> ---
> Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 6 ++++++
> arch/arm/boot/dts/armada-370-xp.dtsi | 2 --
> arch/arm/boot/dts/armada-370.dtsi | 8 ++++++++
> arch/arm/boot/dts/armada-xp.dtsi | 10 ++++++++++
> 4 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> index f46d928..8ede3e7 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> @@ -8,6 +8,12 @@ Required properties :
> - interrupts : The interrupt number
> - clock-frequency : Desired I2C bus clock frequency in Hz.
>
> +Optional properties :
> +
> +- i2c,i2c-bridge : This flag indicate that the i2c controller have the
> + Transaction Generator support and we want to use it. Not all the
> + mv64xxx controller have this feature.
Do you have a list of which controllers definitely do, or definitely
don't? That would be helpful for folks adding new boards.
> +
> Examples:
nit. should the example be updated?
thx,
Jason.
Dear Jason Cooper,
On Fri, 7 Jun 2013 14:09:41 -0400, Jason Cooper wrote:
> > +- i2c,i2c-bridge : This flag indicate that the i2c controller have the
> > + Transaction Generator support and we want to use it. Not all the
> > + mv64xxx controller have this feature.
>
> Do you have a list of which controllers definitely do, or definitely
> don't? That would be helpful for folks adding new boards.
As mentioned in the first commit log:
""
This feature is currently only available on Armada XP, so usage of
this mechanism is activated through device tree.
""
So Armada XP is the only to have that, for now. We can probably assume
future Marvell SoC may also have this feature, but we don't know for
sure.
Best regards,
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
On Fri, Jun 07, 2013 at 05:42:23PM +0200, Gregory CLEMENT wrote:
> The mv64xxx-i2c embedded in the Armada XP have a new feature called
> i2c-bridge. This commit split the i2c information into armada-370.dtsi
> and armada-xp.dtsi. Most of the data remains the same and stay in the
> common file Armada-370-xp.dtsi. With this new feature the size of the
> registers are bigger for Armada XP and for this SoCs we add a new flag
> for the i2c-bridge capability.
Personally, I don't like this "i2c-bridge" flag either, but for a different
reason - i2c-bridge makes it sound like it's doing something it's not.
What it is doing is as you stated in the cover - it's an offload mechanism,
just like if it were possible to use DMA to supply the data to the I2C
interface.
I think Maxime is correct - this should be identified by a variation in
the compatible string, not by a flag in DT.
On Fri, Jun 07, 2013 at 05:42:22PM +0200, Gregory CLEMENT wrote:
> From: Zbigniew Bodek <[email protected]>
>
> 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.
>
> [[email protected]: Rewrite some part to be applied
> on the the code modified by Russell King's fixes]
> [[email protected]: Merge and split the commits to
> push them to the accurate maintainer i2c and of]]
> [[email protected]: Reword the commit log]
>
> Signed-off-by: Piotr Ziecik <[email protected]>
> Signed-off-by: Gregory CLEMENT <[email protected]>
> ---
> drivers/i2c/busses/i2c-mv64xxx.c | 143 +++++++++++++++++++++++++++++++++++++--
> 1 file changed, 139 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index 6356439..5376dc3 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -24,7 +24,7 @@
> #include <linux/clk.h>
> #include <linux/err.h>
>
> -/* Register defines */
> +/* Register defines (I2C port) */
> #define MV64XXX_I2C_REG_SLAVE_ADDR 0x00
> #define MV64XXX_I2C_REG_DATA 0x04
> #define MV64XXX_I2C_REG_CONTROL 0x08
> @@ -59,6 +59,29 @@
> #define MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK 0xe8
> #define MV64XXX_I2C_STATUS_NO_STATUS 0xf8
>
> +/* Register defines (I2C bridge) */
> +#define MV64XXX_I2C_REG_TX_DATA_LO 0xC0
> +#define MV64XXX_I2C_REG_TX_DATA_HI 0xC4
> +#define MV64XXX_I2C_REG_RX_DATA_LO 0xC8
> +#define MV64XXX_I2C_REG_RX_DATA_HI 0xCC
> +#define MV64XXX_I2C_REG_BRIDGE_CONTROL 0xD0
> +#define MV64XXX_I2C_REG_BRIDGE_STATUS 0xD4
> +#define MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE 0xD8
> +#define MV64XXX_I2C_REG_BRIDGE_INTR_MASK 0xDC
> +#define MV64XXX_I2C_REG_BRIDGE_TIMING 0xE0
> +
> +/* Bridge Control values */
> +#define MV64XXX_I2C_BRIDGE_CONTROL_WR 0x00000001
> +#define MV64XXX_I2C_BRIDGE_CONTROL_RD 0x00000002
> +#define MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT 2
> +#define MV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT 0x00001000
> +#define MV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT 13
> +#define MV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT 16
> +#define MV64XXX_I2C_BRIDGE_CONTROL_ENABLE 0x00080000
> +
> +/* Bridge Status values */
> +#define MV64XXX_I2C_BRIDGE_STATUS_ERROR 0x00000001
> +
> /* Driver states */
> enum {
> MV64XXX_I2C_STATE_INVALID,
> @@ -110,6 +133,7 @@ struct mv64xxx_i2c_data {
> spinlock_t lock;
> struct i2c_msg *msg;
> struct i2c_adapter adapter;
> + int bridge_enabled;
> };
>
> static void
> @@ -157,6 +181,16 @@ mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
> writel(0, drv_data->reg_base + MV64XXX_I2C_REG_EXT_SLAVE_ADDR);
> writel(MV64XXX_I2C_REG_CONTROL_TWSIEN | MV64XXX_I2C_REG_CONTROL_STOP,
> drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
> +
> + if (drv_data->bridge_enabled) {
> + writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
> + writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_TIMING);
> + writel(0, drv_data->reg_base +
> + MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
> + writel(0, drv_data->reg_base +
> + MV64XXX_I2C_REG_BRIDGE_INTR_MASK);
> + }
> +
> drv_data->state = MV64XXX_I2C_STATE_IDLE;
> }
>
> @@ -368,6 +402,19 @@ mv64xxx_i2c_intr(int irq, void *dev_id)
> irqreturn_t rc = IRQ_NONE;
>
> spin_lock_irqsave(&drv_data->lock, flags);
> +
> + if (drv_data->bridge_enabled) {
> + if (readl(drv_data->reg_base +
> + MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE)) {
> + writel(0, drv_data->reg_base +
> + MV64XXX_I2C_REG_BRIDGE_CONTROL);
> + writel(0, drv_data->reg_base +
> + MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
> + drv_data->block = 0;
> + wake_up(&drv_data->waitq);
> + rc = IRQ_HANDLED;
> + }
> + }
> while (readl(drv_data->reg_base + MV64XXX_I2C_REG_CONTROL) &
> MV64XXX_I2C_REG_CONTROL_IFLG) {
> status = readl(drv_data->reg_base + MV64XXX_I2C_REG_STATUS);
> @@ -446,6 +493,81 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
> return drv_data->rc;
> }
>
> +static int
> +mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg)
> +{
> + unsigned long data_reg_hi = 0;
> + unsigned long data_reg_lo = 0;
> + unsigned long status_reg;
> + unsigned long ctrl_reg;
> + unsigned long flags;
> + unsigned int i;
> +
> + /* Only regular transactions can be offloaded */
> + if ((msg->flags & ~(I2C_M_TEN | I2C_M_RD)) != 0)
> + return 1;
> +
> + /* Only 1-8 byte transfers can be offloaded */
> + if (msg->len < 1 || msg->len > 8)
> + return 1;
> +
> + /* Build transaction */
> + ctrl_reg = MV64XXX_I2C_BRIDGE_CONTROL_ENABLE |
> + (msg->addr << MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT);
> +
> + if ((msg->flags & I2C_M_TEN) != 0)
> + ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT;
> +
> + if ((msg->flags & I2C_M_RD) == 0) {
> + for (i = 0; i < 4 && i < msg->len; i++)
> + data_reg_lo = data_reg_lo |
> + (msg->buf[i] << ((i & 0x3) * 8));
> +
> + for (i = 4; i < 8 && i < msg->len; i++)
> + data_reg_hi = data_reg_hi |
> + (msg->buf[i] << ((i & 0x3) * 8));
> +
> + ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_WR |
> + (msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT;
> + } else {
> + ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_RD |
> + (msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT;
> + }
> +
> + /* Execute transaction */
> + spin_lock_irqsave(&drv_data->lock, flags);
> + drv_data->block = 1;
> + writel(data_reg_lo, drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_LO);
> + writel(data_reg_hi, drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_HI);
You can use writel_relaxed with the above two writes - you don't need any
synchronization with memory as you aren't doing DMA.
> + writel(ctrl_reg, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
> + spin_unlock_irqrestore(&drv_data->lock, flags);
You can also probably dispense with the spinlock too - the interface should
be idle before you start writing to these registers, and nothing should start
happening until after this last write. Moreover, the I2C core layer will
ensure that there can't be two concurrent transactions.
> +
> + mv64xxx_i2c_wait_for_completion(drv_data);
> +
> + spin_lock_irqsave(&drv_data->lock, flags);
> + status_reg = readl(drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_STATUS);
> + data_reg_lo = readl(drv_data->reg_base + MV64XXX_I2C_REG_RX_DATA_LO);
> + data_reg_hi = readl(drv_data->reg_base + MV64XXX_I2C_REG_RX_DATA_HI);
> + spin_unlock_irqrestore(&drv_data->lock, flags);
And same here - after the completion returns, the interface should now be
idle again, so there should be no concurrency.
> -
> - rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[0], num == 1);
> + if (drv_data->bridge_enabled)
> + rc = mv64xxx_i2c_offload_msg(drv_data, &msgs[0]);
Note that you're only dealing with the first message, so this is buggy.
mv64xxx_i2c_execute_msg() below now deals with the entire transaction
(so all the i2c_msg structures) in one go from interrupt context,
unlike prior to my patch where messages were dealt with one at a time
(and thereby buggily too!)