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 to convert it to
be included in match_data.
> @@ -654,6 +839,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
> drv_data->freq_n = pdata->freq_n;
> drv_data->irq = platform_get_irq(pd, 0);
> drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
> + drv_data->offload_enabled = 0;
'false' instead of 0.
Regards,
Wolfram
signature.asc
Description: Digital signature
