From: gregory.clement@free-electrons•com (Gregory CLEMENT)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v5 1/3] i2c-mv64xxx: Add I2C Transaction Generator support
Date: Thu, 22 Aug 2013 09:40:45 +0200 [thread overview]
Message-ID: <5215C07D.7040000@free-electrons.com> (raw)
In-Reply-To: <20130821210116.GA3130@katana>
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,
>> 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.
>
OK
Thanks for your review.
Gregory
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
next prev parent reply other threads:[~2013-08-22 7:40 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-09 9:05 [PATCH v5 0/3] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP Gregory CLEMENT
2013-08-09 9:05 ` [PATCH v5 1/3] i2c-mv64xxx: Add I2C Transaction Generator support Gregory CLEMENT
2013-08-15 12:13 ` Wolfram Sang
2013-08-20 15:56 ` Gregory CLEMENT
2013-08-21 21:01 ` Wolfram Sang
2013-08-22 7:40 ` Gregory CLEMENT [this message]
2013-08-22 9:06 ` Wolfram Sang
2013-08-09 9:05 ` [PATCH v5 2/3] i2c-mv64xxx: Fix timing issue on Armada XP (errata FE-8471889) Gregory CLEMENT
2013-08-09 9:05 ` [PATCH v5 3/3] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c Gregory CLEMENT
2013-08-09 9:13 ` Ezequiel Garcia
2013-08-09 9:18 ` Gregory CLEMENT
2013-08-09 9:30 ` Ezequiel Garcia
2013-08-09 11:32 ` Jason Cooper
2013-08-20 16:21 ` [PATCH v5 0/3] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP Gregory CLEMENT
2013-08-20 19:07 ` Wolfram Sang
2013-08-21 10:44 ` Gregory CLEMENT
2013-08-21 13:43 ` Wolfram Sang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5215C07D.7040000@free-electrons.com \
--to=gregory.clement@free-electrons$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox