Thu, Oct 10, 2024 at 03:53:16PM +1300, Aryan Srivastava kirjoitti:
> 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.
...
> +/**
> + * octeon_i2c_hlc_block_comp_read - high-level-controller composite block read
> + * @i2c: The struct octeon_i2c
> + * @msgs: msg[0] contains address, place read data into msg[1]
> + *
> + * i2c core command is constructed and written into the SW_TWSI register.
> + * The execution of the command will result in requested data being
> + * placed into a FIFO buffer, ready to be read.
> + * Used in the case where the i2c xfer is for greater than 8 bytes of read data.
> + * Returns 0 on success, otherwise a negative errno.
Please, validate the kernel-doc properly. This has a warning.
> + */
> +static int octeon_i2c_hlc_block_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs)
> +{
> + int len, ret = 0;
> + u64 cmd = 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_R | SW_TWSI_SOVR;
> + cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT;
Can you, please, remove 10-bit address "support" from the driver to avoid false
impression that it works. I haven't found any evidence that the upper 3 bits
are being anyhow used in it.
> + /* 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 (int i = 0; i < len; i += 8) {
> + u64 rd = __raw_readq(i2c->twsi_base + OCTEON_REG_BLOCK_FIFO(i2c));
> + /* Place data into msg buf from FIFO, MSB onwards */
> + for (int j = 7; j >= 0; j--)
> + msgs[1].buf[i + (7 - j)] = (rd >> (8 * j)) & 0xff;
Use proper put_unaligned_be64() / put_unalined_le64() depending on what you
need. No reinveted wheels, please.
> + }
> +
> + octeon_i2c_block_disable(i2c);
> + return ret;
> +}
...
> +/**
> + * octeon_i2c_hlc_block_comp_write - high-level-controller composite block write
> + * @i2c: The struct octeon_i2c
> + * @msgs: msg[0] contains address, msg[1] contains data to be written
> + *
> + * i2c core command is constructed and write data is written into the FIFO buffer.
> + * The execution of the command will result in HW write, using the data in FIFO.
> + * Used in the case where the i2c xfer is for greater than 8 bytes of write data.
> + *
> + * Returns 0 on success, otherwise a negative errno.
Same issue with the kernel-doc.
> + */
> +static int octeon_i2c_hlc_block_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msgs)
> +{
> + bool set_ext = false;
> + int len, ret = 0;
> + 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;
> + cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT;
Same issue with 10-bit address.
> + /* 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 (int i = 0; i < len; i += 8) {
> + u64 buf = 0;
> + /* Place data from msg buf into FIFO, MSB onwards */
> + for (int j = 7; j >= 0; j--)
> + buf |= (msgs[1].buf[i + (7 - j)] << (8 * j));
Same issue with unaligned accesses, use get_unaligned_*64().
> + 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
