| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75VceyOKq1FJkWLg3wUEW-QMEhC9ffJ9JW3V7NaO4zxNvpA@mail.gmail.com>
Date: Tue, 11 Mar 2025 21:45:53 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Aryan Srivastava <aryan.srivastava@...iedtelesis.co.nz>
Cc: Andi Shyti <andi.shyti@...nel.org>, Markus Elfring <Markus.Elfring@....de>,
linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
Robert Richter <rric@...nel.org>
Subject: Re: [PATCH v12 3/3] i2c: octeon: add block-mode i2c operations
On Tue, Mar 11, 2025 at 4:34 AM Aryan Srivastava
<aryan.srivastava@...iedtelesis.co.nz> wrote:
>
> Add functions to perform block read and write operations. This applies
> for cases where the requested operation is for >8 bytes of data.
>
> When not using the block mode transfer, the driver will attempt a series
> of 8 byte i2c operations until it reaches the desired total. For
> example, for a 40 byte request the driver will complete 5 separate
> transactions. This results in large transactions taking a significant
> amount of time to process.
>
> Add block mode such that the driver can request larger transactions, up
> to 1024 bytes per transfer.
>
> Many aspects of the block mode transfer is common with the regular 8
> byte operations. Use generic functions for parts of the message
> construction and sending the message. The key difference for the block
> mode is the usage of separate FIFO buffer to store data.
>
> Write to this buffer in the case of a write (before command send).
> Read from this buffer in the case of a read (after command send).
>
> Data is written into this buffer by placing data into the MSB onwards.
> This means the bottom 8 bits of the data will match the top 8 bits, and
> so on and so forth.
>
> Set specific bits in message for block mode, enable block mode transfers
> from global i2c management registers, construct message, send message,
> read or write from FIFO buffer as required.
>
> The block-mode transactions result in a significant speed increase in
> large i2c requests.
...
> +static int octeon_i2c_hlc_block_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs)
> +{
> + int ret = 0;
Why the useless assignment?
> + u16 len;
> + u64 cmd;
> +
> + octeon_i2c_hlc_enable(i2c);
> + octeon_i2c_block_enable(i2c);
> +
> + /* Write (size - 1) into block control register */
> + len = msgs[1].len - 1;
> + octeon_i2c_writeq_flush((u64)(len), i2c->twsi_base + OCTEON_REG_BLOCK_CTL(i2c));
Why explicit casting? (And no need to have parentheses for simple
cases like this)
> + /* Prepare core command */
> + cmd = SW_TWSI_V | SW_TWSI_R | SW_TWSI_SOVR | SW_TWSI_OP_7_IA;
> + cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT;
> +
> + /* Send core command */
> + ret = octeon_i2c_hlc_read_cmd(i2c, msgs[0], cmd);
> + if (ret)
> + return ret;
> +
> + cmd = __raw_readq(i2c->twsi_base + OCTEON_REG_SW_TWSI(i2c));
> + if ((cmd & SW_TWSI_R) == 0)
> + return octeon_i2c_check_status(i2c, false);
> +
> + /* read data in FIFO */
> + octeon_i2c_writeq_flush(TWSX_BLOCK_STS_RESET_PTR,
> + i2c->twsi_base + OCTEON_REG_BLOCK_STS(i2c));
> + for (u16 i = 0; i <= len; i += 8) {
> + /* Byte-swap FIFO data and copy into msg buffer */
> + u64 rd = cpu_to_be64(__raw_readq(i2c->twsi_base + OCTEON_REG_BLOCK_FIFO(i2c)));
> +
> + memcpy(&msgs[1].buf[i], &rd, MIN_T(u16, 8, msgs[1].len - i));
Why MIN_T()?! umin() or min() should work.
> + }
> +
> + octeon_i2c_block_disable(i2c);
> + return ret;
> +}
...
> +static int octeon_i2c_hlc_block_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msgs)
> +{
Same comments as per above.
> + bool set_ext = false;
> + int ret = 0;
Why do you need these assignments? What for?
> + u16 len;
> + u64 cmd, ext = 0;
> +
> + octeon_i2c_hlc_enable(i2c);
> + octeon_i2c_block_enable(i2c);
> +
> + /* Write (size - 1) into block control register */
> + len = msgs[1].len - 1;
> + octeon_i2c_writeq_flush((u64)(len), i2c->twsi_base + OCTEON_REG_BLOCK_CTL(i2c));
> +
> + /* Prepare core command */
> + cmd = SW_TWSI_V | SW_TWSI_SOVR | SW_TWSI_OP_7_IA;
> + cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT;
> +
> + /* Set parameters for extended message (if required) */
> + set_ext = octeon_i2c_hlc_ext(i2c, msgs[0], &cmd, &ext);
> +
> + /* Write msg into FIFO buffer */
> + octeon_i2c_writeq_flush(TWSX_BLOCK_STS_RESET_PTR,
> + i2c->twsi_base + OCTEON_REG_BLOCK_STS(i2c));
> + for (u16 i = 0; i <= len; i += 8) {
> + u64 buf = 0;
> +
> + /* Copy 8 bytes or remaining bytes from message buffer */
> + memcpy(&buf, &msgs[1].buf[i], MIN_T(u16, 8, msgs[1].len - i));
> +
> + /* Byte-swap message data and write into FIFO */
> + buf = cpu_to_be64(buf);
> + octeon_i2c_writeq_flush(buf, i2c->twsi_base + OCTEON_REG_BLOCK_FIFO(i2c));
> + }
> + if (set_ext)
> + octeon_i2c_writeq_flush(ext, i2c->twsi_base + OCTEON_REG_SW_TWSI_EXT(i2c));
> +
> + /* Send command to core (send data in FIFO) */
> + ret = octeon_i2c_hlc_cmd_send(i2c, cmd);
> + if (ret)
> + return ret;
> +
> + cmd = __raw_readq(i2c->twsi_base + OCTEON_REG_SW_TWSI(i2c));
> + if ((cmd & SW_TWSI_R) == 0)
> + return octeon_i2c_check_status(i2c, false);
> +
> + octeon_i2c_block_disable(i2c);
> + return ret;
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists