Received-SPF: pass (google.com: domain of linux-kernel+bounces-145959-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 15 Apr 2024 23:59:35 +0200
From: Andi Shyti <andi.shyti@kernel.org>
To: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>
Cc: linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] i2c: octeon: Add block-mode r/w
Message-ID: <o6xuyvunkceihtx4aifryfwviedx26scmlahygw5blijodmtge@c5cyhfoez5qq>
References: <20230905102234.nlaeskxbbvu74co2@zenone.zhora.eu>
 <20240415005213.3477671-1-aryan.srivastava@alliedtelesis.co.nz>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240415005213.3477671-1-aryan.srivastava@alliedtelesis.co.nz>

Hi Aryan,

On Mon, Apr 15, 2024 at 12:52:13PM +1200, Aryan Srivastava wrote:
> Add support for block mode read/write operations on
> Thunderx chips.
> 
> When attempting r/w operations of greater then 8 bytes
> block mode is used, instead of performing a series of
> 8 byte reads.

Can you please add some more description of your patch here.

How did you do it? Which modes have you added? What are these
modes doing and how they work?

The patch is not the easiest itself and with little description
is very challenging to review. Please make my life easier :-)

> Signed-off-by: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>

..

> +static void octeon_i2c_block_enable(struct octeon_i2c *i2c)
> +{
> +	if (i2c->block_enabled || !TWSI_BLOCK_CTL(i2c))

does the block_ctl register stores the length of the message?
If it goes '0' does it mean that it's ready for a block transfer?
(same question for the disable function).

> +		return;
> +
> +	i2c->block_enabled = true;
> +	octeon_i2c_writeq_flush(TWSI_MODE_STRETCH
> +		| TWSI_MODE_BLOCK_MODE, i2c->twsi_base + TWSI_MODE(i2c));
> +}

..

> @@ -579,10 +612,7 @@ static int octeon_i2c_hlc_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msg
>  	if (set_ext)
>  		octeon_i2c_writeq_flush(ext, i2c->twsi_base + SW_TWSI_EXT(i2c));
>  
> -	octeon_i2c_hlc_int_clear(i2c);
> -	octeon_i2c_writeq_flush(cmd, i2c->twsi_base + SW_TWSI(i2c));
> -
> -	ret = octeon_i2c_hlc_wait(i2c);
> +	ret = octeon_i2c_hlc_cmd_send(i2c, cmd);
>  	if (ret)
>  		goto err;

Can you put the octeon_i2c_hlc_comp_read/octeon_i2c_hlc_comp_write
refactoring in a different patch as a preparatory to this one?
It's easier to review.

Please, remember to keep patches logically separated in smaller
chunks.

>  
> @@ -594,6 +624,106 @@ static int octeon_i2c_hlc_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msg
>  	return ret;
>  }
>  
> +/**
> + * high-level-controller composite block write+read, msg0=addr, msg1=data

This message doesn't mean much. Please check the DOC formatting
and the other functions, as well.

Remember good comments are highly appreciated.

> + * Used in the case where the i2c xfer is for greater than 8 bytes of read data.
> + */

..

> +	/* read data in FIFO */
> +	octeon_i2c_writeq_flush(TWSI_BLOCK_STS_RESET_PTR, i2c->twsi_base + TWSI_BLOCK_STS(i2c));
> +	for (int i = 0; i < len; i += 8) {
> +		u64 rd = __raw_readq(i2c->twsi_base + TWSI_BLOCK_FIFO(i2c));
> +		for (int j = 7; j >= 0; j--)
> +			msgs[1].buf[i + (7 - j)] = (rd >> (8 * j)) & 0xff;

Looks good, but do you mind a comment here?

> +	}
> +
> +	octeon_i2c_block_disable(i2c);
> +	return ret;
> +}

..

> +	/* Write msg into FIFO buffer */
> +	octeon_i2c_writeq_flush(TWSI_BLOCK_STS_RESET_PTR, i2c->twsi_base + TWSI_BLOCK_STS(i2c));
> +	for (int i = 0; i < len; i += 8) {
> +		u64 buf = 0;
> +		for (int j = 7; j >= 0; j--)
> +			buf |= (msgs[1].buf[i + (7 - j)] << (8 * j));

a comment here?

> +		octeon_i2c_writeq_flush(buf, i2c->twsi_base + TWSI_BLOCK_FIFO(i2c));
> +	}
> +	if (set_ext)
> +		octeon_i2c_writeq_flush(ext, i2c->twsi_base + SW_TWSI_EXT(i2c));
> +
> +	/* Send command to core (send data in FIFO) */
> +	ret = octeon_i2c_hlc_cmd_send(i2c, cmd);
> +	if (ret)
> +		return ret;

do we need to disable anything here?

Thanks for your patch,
Andi

> +
> +	cmd = __raw_readq(i2c->twsi_base + SW_TWSI(i2c));
> +	if ((cmd & SW_TWSI_R) == 0)
> +		return octeon_i2c_check_status(i2c, false);
> +
> +	octeon_i2c_block_disable(i2c);
> +	return ret;
> +}

..