Re: [PATCH v10 2/2] i2c: octeon: Add block-mode i2c operations

[Andy Shevchenko]

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