Re: [PATCH v5 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
> >> @@ -326,6 +463,12 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data > >> *drv_data) > >>drv_data->reg_base + drv_data->reg_offsets.control); > >>break; > >> > >> + case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START: > >> + if (mv64xxx_i2c_offload_msg(drv_data) <= 0) > > > > needs to be adjusted when using -EINVAL above. I'd prefer the error case > > in the else branch, though. Easier to read. > > > > OK, but in this case ... > > >> + break; > >> + else > >> + drv_data->action = MV64XXX_I2C_ACTION_SEND_START; > >> + /* FALLTHRU */ > > ... the fall through here is less readable. But it is a matter of > taste, I will change this. Ah, I see. Well, try both and decide. Thanks! signature.asc Description: Digital signature
Re: [PATCH v5 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
On 21/08/2013 23:01, Wolfram Sang wrote:
> Hi,
>
> here is the review. BTW have you tested with and without the offload
> engine?
yes with eeprog.
>
>> +static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
>> +{
>> +unsigned long data_reg_hi = 0;
>> +unsigned long data_reg_lo = 0;
>> +unsigned long ctrl_reg;
>> +unsigned int i;
>> +struct i2c_msg *msg = drv_data->msgs;
>> +
>> +drv_data->msg = msg;
>> +drv_data->byte_posn = 0;
>> +drv_data->bytes_left = msg->len;
>> +drv_data->aborting = 0;
>> +drv_data->rc = 0;
>> +/* Only regular transactions can be offloaded */
>> +if ((msg->flags & ~(I2C_M_TEN | I2C_M_RD)) != 0)
>> +return 1;
>
> -EINVAL?
>
OK
>> +
>> +/* Only 1-8 byte transfers can be offloaded */
>> +if (msg->len < 1 || msg->len > 8)
>> +return 1;
>
> ditto
>
OK
>> +
>> +/* Build transaction */
>> +ctrl_reg = MV64XXX_I2C_BRIDGE_CONTROL_ENABLE |
>> + (msg->addr << MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT);
>> +
>> +if ((msg->flags & I2C_M_TEN) != 0)
>> +ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT;
>> +
>> +if ((msg->flags & I2C_M_RD) == 0) {
>> +for (i = 0; i < 4 && i < msg->len; i++)
>> +data_reg_lo = data_reg_lo |
>> +(msg->buf[i] << ((i & 0x3) * 8));
>> +
>> +for (i = 4; i < 8 && i < msg->len; i++)
>> +data_reg_hi = data_reg_hi |
>> +(msg->buf[i] << ((i & 0x3) * 8));
>
> What about:
>
> local_buf[8] = { 0 };
>
> memcpy(local_buf, msg->buf, msg->len);
> cpu_to_be32(...)
>
> ? A lot less lines and be32 macros are likely more efficient. Copy loop
> probably, too.
>
OK
>> +
>> +ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_WR |
>> +(msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT;
>> +} else {
>> +ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_RD |
>> +(msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT;
>> +}
>> +
>> +/* Execute transaction */
>> +writel_relaxed(data_reg_lo,
>> +drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_LO);
>> +writel_relaxed(data_reg_hi,
>> +drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_HI);
>
> Do you need to write the 0 in case of I2C_M_RD?
>
Not sure I will check it
>> +writel(ctrl_reg, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
>> +
>> +return 0;
>> +}
>> +
>> +static void
>> +mv64xxx_i2c_update_offload_data(struct i2c_msg *msg, unsigned long
>> data_reg_hi,
>> +unsigned long data_reg_lo)
>> +{
>> +int i;
>> +
>> +if ((msg->flags & I2C_M_RD) != 0) {
>
> != 0 is superfluous
>
OK
>> +for (i = 0; i < 4 && i < msg->len; i++) {
>> +msg->buf[i] = data_reg_lo & 0xFF;
>> +data_reg_lo >>= 8;
>> +}
>> +
>> +for (i = 4; i < 8 && i < msg->len; i++) {
>> +msg->buf[i] = data_reg_hi & 0xFF;
>> +data_reg_hi >>= 8;
>> +}
>> +}
>
> Same idea as above?
>
OK I will do it here also as in both cases the number of lines will be
shorter, but for small amount of data I am not sure it will be faster.
>> @@ -298,21 +420,36 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32
>> status)
>> static void
>> mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>> {
>> +unsigned long data_reg_hi = 0;
>> +unsigned long data_reg_lo = 0;
>> +
>> switch(drv_data->action) {
>> +case MV64XXX_I2C_ACTION_OFFLOAD_RESTART:
>> +data_reg_lo = readl(drv_data->reg_base +
>> +MV64XXX_I2C_REG_RX_DATA_LO);
>> +data_reg_hi = readl(drv_data->reg_base +
>> +MV64XXX_I2C_REG_RX_DATA_HI);
>
> Initializing data_reg_* is the same for both calls to
> update_offload_data, so it could be moved into the function.
> Probably not needed when using the local_buf idea.
>
I will move this part in the update_offload_data function.
>> @@ -326,6 +463,12 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>> drv_data->reg_base + drv_data->reg_offsets.control);
>> break;
>>
>> +case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
>> +if (mv64xxx_i2c_offload_msg(drv_data) <= 0)
>
> needs to be adjusted when using -EINVAL above. I'd prefer the error case
> in the else branch, though. Easier to read.
>
OK, but in this case ...
>> +break;
>> +else
>> +drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
>> +/* FALLTHRU */
... the fall through here is less readable. But it is a matter of
taste, I will change this.
>> case MV64XXX_I2C_ACTION_SEND_START:
>> writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
>>
Re: [PATCH v5 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
Hi,
here is the review. BTW have you tested with and without the offload
engine?
> +static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
> +{
> + unsigned long data_reg_hi = 0;
> + unsigned long data_reg_lo = 0;
> + unsigned long ctrl_reg;
> + unsigned int i;
> + struct i2c_msg *msg = drv_data->msgs;
> +
> + drv_data->msg = msg;
> + drv_data->byte_posn = 0;
> + drv_data->bytes_left = msg->len;
> + drv_data->aborting = 0;
> + drv_data->rc = 0;
> + /* Only regular transactions can be offloaded */
> + if ((msg->flags & ~(I2C_M_TEN | I2C_M_RD)) != 0)
> + return 1;
-EINVAL?
> +
> + /* Only 1-8 byte transfers can be offloaded */
> + if (msg->len < 1 || msg->len > 8)
> + return 1;
ditto
> +
> + /* Build transaction */
> + ctrl_reg = MV64XXX_I2C_BRIDGE_CONTROL_ENABLE |
> +(msg->addr << MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT);
> +
> + if ((msg->flags & I2C_M_TEN) != 0)
> + ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT;
> +
> + if ((msg->flags & I2C_M_RD) == 0) {
> + for (i = 0; i < 4 && i < msg->len; i++)
> + data_reg_lo = data_reg_lo |
> + (msg->buf[i] << ((i & 0x3) * 8));
> +
> + for (i = 4; i < 8 && i < msg->len; i++)
> + data_reg_hi = data_reg_hi |
> + (msg->buf[i] << ((i & 0x3) * 8));
What about:
local_buf[8] = { 0 };
memcpy(local_buf, msg->buf, msg->len);
cpu_to_be32(...)
? A lot less lines and be32 macros are likely more efficient. Copy loop
probably, too.
> +
> + ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_WR |
> + (msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT;
> + } else {
> + ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_RD |
> + (msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT;
> + }
> +
> + /* Execute transaction */
> + writel_relaxed(data_reg_lo,
> + drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_LO);
> + writel_relaxed(data_reg_hi,
> + drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_HI);
Do you need to write the 0 in case of I2C_M_RD?
> + writel(ctrl_reg, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
> +
> + return 0;
> +}
> +
> +static void
> +mv64xxx_i2c_update_offload_data(struct i2c_msg *msg, unsigned long
> data_reg_hi,
> + unsigned long data_reg_lo)
> +{
> + int i;
> +
> + if ((msg->flags & I2C_M_RD) != 0) {
!= 0 is superfluous
> + for (i = 0; i < 4 && i < msg->len; i++) {
> + msg->buf[i] = data_reg_lo & 0xFF;
> + data_reg_lo >>= 8;
> + }
> +
> + for (i = 4; i < 8 && i < msg->len; i++) {
> + msg->buf[i] = data_reg_hi & 0xFF;
> + data_reg_hi >>= 8;
> + }
> + }
Same idea as above?
> @@ -298,21 +420,36 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32
> status)
> static void
> mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
> {
> + unsigned long data_reg_hi = 0;
> + unsigned long data_reg_lo = 0;
> +
> switch(drv_data->action) {
> + case MV64XXX_I2C_ACTION_OFFLOAD_RESTART:
> + data_reg_lo = readl(drv_data->reg_base +
> + MV64XXX_I2C_REG_RX_DATA_LO);
> + data_reg_hi = readl(drv_data->reg_base +
> + MV64XXX_I2C_REG_RX_DATA_HI);
Initializing data_reg_* is the same for both calls to
update_offload_data, so it could be moved into the function.
Probably not needed when using the local_buf idea.
> @@ -326,6 +463,12 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
> drv_data->reg_base + drv_data->reg_offsets.control);
> break;
>
> + case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
> + if (mv64xxx_i2c_offload_msg(drv_data) <= 0)
needs to be adjusted when using -EINVAL above. I'd prefer the error case
in the else branch, though. Easier to read.
> + break;
> + else
> + drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
> + /* FALLTHRU */
> case MV64XXX_I2C_ACTION_SEND_START:
> writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
> drv_data->reg_base + drv_data->reg_offsets.control);
> @@ -601,6 +779,13 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
>
> memcpy(&drv_data->reg_offsets, device->data,
> sizeof(drv_data->reg_offsets));
>
> + /*
> + * For controllers embedded in new SoCs activate the
> + * Transaction Generator support.
> + */
> + if (of_device_is_compatible(np, "marvell,mv78230-i2c"))
> + drv_data->offload_enabled = true;
For now OK, if there are more users, someone will need t
Re: [PATCH v5 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
On 15/08/2013 14:13, Wolfram Sang wrote:
>
>> +if ((msg->flags & I2C_M_RD) == 0) { + for (i = 0; i < 4 && i
>> < msg->len; i++) + data_reg_lo = data_reg_lo | +
>> (msg->buf[i] << ((i & 0x3) * 8)); + +
>> for (i = 4; i < 8 && i <
>> msg->len; i++) + data_reg_hi = data_reg_hi | +
>> (msg->buf[i] << ((i & 0x3) * 8));
>
> Same comment as in the last version: What about be32_to_cpu and friends
> instead of the loops (here and later)?
>
We can't use be32_to_cpu and friends, because one of the stop condition of this
loop is the length of the msg.
For reading from msg->buf we could consider to mask the irrelevant part.But for
writing to msg->buf it would imply to write outside of the allocated buffer!
Do you agree to not change anything on this patch?
Thanks,
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
> + if ((msg->flags & I2C_M_RD) == 0) {
> + for (i = 0; i < 4 && i < msg->len; i++)
> + data_reg_lo = data_reg_lo |
> + (msg->buf[i] << ((i & 0x3) * 8));
> +
> + for (i = 4; i < 8 && i < msg->len; i++)
> + data_reg_hi = data_reg_hi |
> + (msg->buf[i] << ((i & 0x3) * 8));
Same comment as in the last version: What about be32_to_cpu and friends
instead of the loops (here and later)?
signature.asc
Description: Digital signature
[PATCH v5 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
The I2C Transaction Generator offloads CPU from managing I2C
transfer step by step.
This feature is currently only available on Armada XP, so usage of
this mechanism is activated through device tree.
Based on the work of Piotr Ziecik and rewrote to use the new way of
handling multiples i2c messages.
Signed-off-by: Piotr Ziecik
Signed-off-by: Gregory CLEMENT
---
drivers/i2c/busses/i2c-mv64xxx.c | 208 ---
1 file changed, 197 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index b1f42bf..406d9c6 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -55,6 +55,32 @@
#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK0xe8
#defineMV64XXX_I2C_STATUS_NO_STATUS0xf8
+/* Register defines (I2C bridge) */
+#defineMV64XXX_I2C_REG_TX_DATA_LO 0xc0
+#defineMV64XXX_I2C_REG_TX_DATA_HI 0xc4
+#defineMV64XXX_I2C_REG_RX_DATA_LO 0xc8
+#defineMV64XXX_I2C_REG_RX_DATA_HI 0xcc
+#defineMV64XXX_I2C_REG_BRIDGE_CONTROL 0xd0
+#defineMV64XXX_I2C_REG_BRIDGE_STATUS 0xd4
+#defineMV64XXX_I2C_REG_BRIDGE_INTR_CAUSE 0xd8
+#defineMV64XXX_I2C_REG_BRIDGE_INTR_MASK0xdC
+#defineMV64XXX_I2C_REG_BRIDGE_TIMING 0xe0
+
+/* Bridge Control values */
+#defineMV64XXX_I2C_BRIDGE_CONTROL_WR 0x0001
+#defineMV64XXX_I2C_BRIDGE_CONTROL_RD 0x0002
+#defineMV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT 2
+#defineMV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT 0x1000
+#defineMV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT13
+#defineMV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT16
+#defineMV64XXX_I2C_BRIDGE_CONTROL_ENABLE 0x0008
+
+/* Bridge Status values */
+#defineMV64XXX_I2C_BRIDGE_STATUS_ERROR 0x0001
+#defineMV64XXX_I2C_STATUS_OFFLOAD_ERROR0xf001
+#defineMV64XXX_I2C_STATUS_OFFLOAD_OK 0xf000
+
+
/* 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;
+ booloffload_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;
