[PATCH v13 0/3] i2c: octeon: add block-mode r/w

[PATCH v13 0/3] i2c: octeon: add block-mode r/w

[Aryan Srivastava]

Add support for block mode read/write operations on
Thunderx chips.

-Refactor common code for i2c transactions (merged in v11).
-Correct function docs.
-Remove 10-bit addressing considerations.
-Add block mode transaction functionality.

Signed-off-by: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>
---
Changes in v2:
- comment style and formatting.

Changes in v3:
- comment style and formatting.

Changes in v4:
- Refactoring common code.
- Additional comments.

Changes in v5:
- Further refactoring.
- Split refactoring into separate patch in series.
- Add more comments + details to comments.

Changes in v6:
- Reword/reformat commit messages

Changes in v7:
- Fix typo in commit message.
- Remove usage of r/w and hlc abbreviations from commits.

Changes in v8:
- Updated refactor commit msg with more information.
- Rebased patch

Changes in v9:
- Rebased patch against i2c-host

Changes in v10:
- Fixed unitialised variable

Changes in v11:
- Rebased patch against i2c-host

Changes in v12:
- Added fixes for function docs
- Removed considerations for 10-bit addressing
- Transition to using appropriate byte-swap operations, instead of custom code
- Refactor commit removed from this set, merged in:
  https://lore.kernel.org/linux-i2c/seom4yspcjnmdxxwn6wuyiqdy2ywpna6nw4rn36tsqinlncbca@jdehzfnznlsg/

Changes in v13:
- Removed unnessary assignments
- Removed explicit casting
- Correct typing for be64 ops
- Use min() instead of MIN_T

Aryan Srivastava (3):
  i2c: octeon: fix return commenting
  i2c: octeon: remove 10-bit addressing support
  i2c: octeon: add block-mode i2c operations

 drivers/i2c/busses/i2c-octeon-core.c     | 195 +++++++++++++++++++----
 drivers/i2c/busses/i2c-octeon-core.h     |  13 +-
 drivers/i2c/busses/i2c-thunderx-pcidrv.c |   3 +
 3 files changed, 179 insertions(+), 32 deletions(-)

-- 
2.47.1

[PATCH v13 1/3] i2c: octeon: fix return commenting

[Aryan Srivastava]

Kernel-docs require a ':' to signify the return behaviour of a function
with within the comment. Many functions in this file were missing ':'
after the "Returns" line, resulting in kernel-doc warnings.

Add the ':' to satisfy kernel-doc requirements.

Signed-off-by: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>
---
 drivers/i2c/busses/i2c-octeon-core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index 3fbc828508ab..0094fe5f7460 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -45,7 +45,7 @@ static bool octeon_i2c_test_iflg(struct octeon_i2c *i2c)
  * octeon_i2c_wait - wait for the IFLG to be set
  * @i2c: The struct octeon_i2c
  *
- * Returns 0 on success, otherwise a negative errno.
+ * Returns: 0 on success, otherwise a negative errno.
  */
 static int octeon_i2c_wait(struct octeon_i2c *i2c)
 {
@@ -139,7 +139,7 @@ static void octeon_i2c_hlc_disable(struct octeon_i2c *i2c)
  * octeon_i2c_hlc_wait - wait for an HLC operation to complete
  * @i2c: The struct octeon_i2c
  *
- * Returns 0 on success, otherwise -ETIMEDOUT.
+ * Returns: 0 on success, otherwise -ETIMEDOUT.
  */
 static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c)
 {
@@ -273,7 +273,7 @@ static int octeon_i2c_recovery(struct octeon_i2c *i2c)
  * octeon_i2c_start - send START to the bus
  * @i2c: The struct octeon_i2c
  *
- * Returns 0 on success, otherwise a negative errno.
+ * Returns: 0 on success, otherwise a negative errno.
  */
 static int octeon_i2c_start(struct octeon_i2c *i2c)
 {
@@ -314,7 +314,7 @@ static void octeon_i2c_stop(struct octeon_i2c *i2c)
  *
  * The address is sent over the bus, then the data is read.
  *
- * Returns 0 on success, otherwise a negative errno.
+ * Returns: 0 on success, otherwise a negative errno.
  */
 static int octeon_i2c_read(struct octeon_i2c *i2c, int target,
 			   u8 *data, u16 *rlength, bool recv_len)
@@ -382,7 +382,7 @@ static int octeon_i2c_read(struct octeon_i2c *i2c, int target,
  *
  * The address is sent over the bus, then the data.
  *
- * Returns 0 on success, otherwise a negative errno.
+ * Returns: 0 on success, otherwise a negative errno.
  */
 static int octeon_i2c_write(struct octeon_i2c *i2c, int target,
 			    const u8 *data, int length)
@@ -625,7 +625,7 @@ static int octeon_i2c_hlc_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msg
  * @msgs: Pointer to the messages to be processed
  * @num: Length of the MSGS array
  *
- * Returns the number of messages processed, or a negative errno on failure.
+ * Returns: the number of messages processed, or a negative errno on failure.
  */
 int octeon_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
-- 
2.47.1

[PATCH v13 2/3] i2c: octeon: remove 10-bit addressing support

[Aryan Srivastava]

The driver gives the illusion of 10-bit address support, but the upper
3 bits of the given address are always thrown away. Remove unnecessary
considerations for 10 bit addressing and always complete 7 bit ops when
using the hlc methods.

Signed-off-by: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>
---
 drivers/i2c/busses/i2c-octeon-core.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index 0094fe5f7460..baf6b27f3752 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -421,17 +421,12 @@ static int octeon_i2c_hlc_read(struct octeon_i2c *i2c, struct i2c_msg *msgs)
 	octeon_i2c_hlc_enable(i2c);
 	octeon_i2c_hlc_int_clear(i2c);
 
-	cmd = SW_TWSI_V | SW_TWSI_R | SW_TWSI_SOVR;
+	cmd = SW_TWSI_V | SW_TWSI_R | SW_TWSI_SOVR | SW_TWSI_OP_7;
 	/* SIZE */
 	cmd |= (u64)(msgs[0].len - 1) << SW_TWSI_SIZE_SHIFT;
 	/* A */
 	cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT;
 
-	if (msgs[0].flags & I2C_M_TEN)
-		cmd |= SW_TWSI_OP_10;
-	else
-		cmd |= SW_TWSI_OP_7;
-
 	octeon_i2c_writeq_flush(cmd, i2c->twsi_base + OCTEON_REG_SW_TWSI(i2c));
 	ret = octeon_i2c_hlc_wait(i2c);
 	if (ret)
@@ -463,17 +458,12 @@ static int octeon_i2c_hlc_write(struct octeon_i2c *i2c, struct i2c_msg *msgs)
 	octeon_i2c_hlc_enable(i2c);
 	octeon_i2c_hlc_int_clear(i2c);
 
-	cmd = SW_TWSI_V | SW_TWSI_SOVR;
+	cmd = SW_TWSI_V | SW_TWSI_SOVR | SW_TWSI_OP_7;
 	/* SIZE */
 	cmd |= (u64)(msgs[0].len - 1) << SW_TWSI_SIZE_SHIFT;
 	/* A */
 	cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT;
 
-	if (msgs[0].flags & I2C_M_TEN)
-		cmd |= SW_TWSI_OP_10;
-	else
-		cmd |= SW_TWSI_OP_7;
-
 	for (i = 0, j = msgs[0].len - 1; i  < msgs[0].len && i < 4; i++, j--)
 		cmd |= (u64)msgs[0].buf[j] << (8 * i);
 
@@ -513,11 +503,6 @@ static bool octeon_i2c_hlc_ext(struct octeon_i2c *i2c, struct i2c_msg msg, u64 *
 	bool set_ext = false;
 	u64 cmd = 0;
 
-	if (msg.flags & I2C_M_TEN)
-		cmd |= SW_TWSI_OP_10_IA;
-	else
-		cmd |= SW_TWSI_OP_7_IA;
-
 	if (msg.len == 2) {
 		cmd |= SW_TWSI_EIA;
 		*ext = (u64)msg.buf[0] << SW_TWSI_IA_SHIFT;
@@ -550,7 +535,7 @@ static int octeon_i2c_hlc_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs
 
 	octeon_i2c_hlc_enable(i2c);
 
-	cmd = SW_TWSI_V | SW_TWSI_R | SW_TWSI_SOVR;
+	cmd = SW_TWSI_V | SW_TWSI_R | SW_TWSI_SOVR | SW_TWSI_OP_7_IA;
 	/* SIZE */
 	cmd |= (u64)(msgs[1].len - 1) << SW_TWSI_SIZE_SHIFT;
 	/* A */
@@ -587,7 +572,7 @@ static int octeon_i2c_hlc_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msg
 
 	octeon_i2c_hlc_enable(i2c);
 
-	cmd = SW_TWSI_V | SW_TWSI_SOVR;
+	cmd = SW_TWSI_V | SW_TWSI_SOVR | SW_TWSI_OP_7_IA;
 	/* SIZE */
 	cmd |= (u64)(msgs[1].len - 1) << SW_TWSI_SIZE_SHIFT;
 	/* A */
-- 
2.47.1

[PATCH v13 3/3] i2c: octeon: add block-mode i2c operations

[Aryan Srivastava]

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.

Signed-off-by: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>
---
 drivers/i2c/busses/i2c-octeon-core.c     | 160 ++++++++++++++++++++++-
 drivers/i2c/busses/i2c-octeon-core.h     |  13 +-
 drivers/i2c/busses/i2c-thunderx-pcidrv.c |   3 +
 3 files changed, 169 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index baf6b27f3752..c22a8f3d78d6 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -135,6 +135,32 @@ static void octeon_i2c_hlc_disable(struct octeon_i2c *i2c)
 	octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB);
 }
 
+static void octeon_i2c_block_enable(struct octeon_i2c *i2c)
+{
+	u64 mode;
+
+	if (i2c->block_enabled || !OCTEON_REG_BLOCK_CTL(i2c))
+		return;
+
+	i2c->block_enabled = true;
+	mode = __raw_readq(i2c->twsi_base + OCTEON_REG_MODE(i2c));
+	mode |= TWSX_MODE_BLOCK_MODE;
+	octeon_i2c_writeq_flush(mode, i2c->twsi_base + OCTEON_REG_MODE(i2c));
+}
+
+static void octeon_i2c_block_disable(struct octeon_i2c *i2c)
+{
+	u64 mode;
+
+	if (!i2c->block_enabled || !OCTEON_REG_BLOCK_CTL(i2c))
+		return;
+
+	i2c->block_enabled = false;
+	mode = __raw_readq(i2c->twsi_base + OCTEON_REG_MODE(i2c));
+	mode &= ~TWSX_MODE_BLOCK_MODE;
+	octeon_i2c_writeq_flush(mode, i2c->twsi_base + OCTEON_REG_MODE(i2c));
+}
+
 /**
  * octeon_i2c_hlc_wait - wait for an HLC operation to complete
  * @i2c: The struct octeon_i2c
@@ -281,6 +307,7 @@ static int octeon_i2c_start(struct octeon_i2c *i2c)
 	u8 stat;
 
 	octeon_i2c_hlc_disable(i2c);
+	octeon_i2c_block_disable(i2c);
 
 	octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB | TWSI_CTL_STA);
 	ret = octeon_i2c_wait(i2c);
@@ -604,6 +631,119 @@ static int octeon_i2c_hlc_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msg
 	return ret;
 }
 
+/**
+ * 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.
+ */
+static int octeon_i2c_hlc_block_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs)
+{
+	int ret;
+	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));
+
+	/* 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 */
+		__be64 rd = cpu_to_be64(__raw_readq(i2c->twsi_base + OCTEON_REG_BLOCK_FIFO(i2c)));
+
+		memcpy(&msgs[1].buf[i], &rd, min(8, msgs[1].len - i));
+	}
+
+	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.
+ */
+static int octeon_i2c_hlc_block_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msgs)
+{
+	bool set_ext;
+	int ret;
+	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) {
+		__be64 buf = 0;
+
+		/* Copy 8 bytes or remaining bytes from message buffer */
+		memcpy(&buf, &msgs[1].buf[i], min(8, msgs[1].len - i));
+
+		/* Byte-swap message data and write into FIFO */
+		buf = cpu_to_be64(buf);
+		octeon_i2c_writeq_flush((u64)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;
+}
+
 /**
  * octeon_i2c_xfer - The driver's xfer function
  * @adap: Pointer to the i2c_adapter structure
@@ -630,13 +770,21 @@ int octeon_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 			if ((msgs[0].flags & I2C_M_RD) == 0 &&
 			    (msgs[1].flags & I2C_M_RECV_LEN) == 0 &&
 			    msgs[0].len > 0 && msgs[0].len <= 2 &&
-			    msgs[1].len > 0 && msgs[1].len <= 8 &&
+			    msgs[1].len > 0 &&
 			    msgs[0].addr == msgs[1].addr) {
-				if (msgs[1].flags & I2C_M_RD)
-					ret = octeon_i2c_hlc_comp_read(i2c, msgs);
-				else
-					ret = octeon_i2c_hlc_comp_write(i2c, msgs);
-				goto out;
+				if (msgs[1].len <= 8) {
+					if (msgs[1].flags & I2C_M_RD)
+						ret = octeon_i2c_hlc_comp_read(i2c, msgs);
+					else
+						ret = octeon_i2c_hlc_comp_write(i2c, msgs);
+					goto out;
+				} else if (msgs[1].len <= 1024 && OCTEON_REG_BLOCK_CTL(i2c)) {
+					if (msgs[1].flags & I2C_M_RD)
+						ret = octeon_i2c_hlc_block_comp_read(i2c, msgs);
+					else
+						ret = octeon_i2c_hlc_block_comp_write(i2c, msgs);
+					goto out;
+				}
 			}
 		}
 	}
diff --git a/drivers/i2c/busses/i2c-octeon-core.h b/drivers/i2c/busses/i2c-octeon-core.h
index b265e21189a1..fecaf7388182 100644
--- a/drivers/i2c/busses/i2c-octeon-core.h
+++ b/drivers/i2c/busses/i2c-octeon-core.h
@@ -96,18 +96,28 @@ struct octeon_i2c_reg_offset {
 	unsigned int twsi_int;
 	unsigned int sw_twsi_ext;
 	unsigned int mode;
+	unsigned int block_ctl;
+	unsigned int block_sts;
+	unsigned int block_fifo;
 };
 
 #define OCTEON_REG_SW_TWSI(x)		((x)->roff.sw_twsi)
 #define OCTEON_REG_TWSI_INT(x)		((x)->roff.twsi_int)
 #define OCTEON_REG_SW_TWSI_EXT(x)	((x)->roff.sw_twsi_ext)
 #define OCTEON_REG_MODE(x)		((x)->roff.mode)
+#define OCTEON_REG_BLOCK_CTL(x)	(x->roff.block_ctl)
+#define OCTEON_REG_BLOCK_STS(x)	(x->roff.block_sts)
+#define OCTEON_REG_BLOCK_FIFO(x)	(x->roff.block_fifo)
 
-/* Set REFCLK_SRC and HS_MODE in TWSX_MODE register */
+/* TWSX_MODE register */
 #define TWSX_MODE_REFCLK_SRC	BIT(4)
+#define TWSX_MODE_BLOCK_MODE	BIT(2)
 #define TWSX_MODE_HS_MODE	BIT(0)
 #define TWSX_MODE_HS_MASK	(TWSX_MODE_REFCLK_SRC | TWSX_MODE_HS_MODE)
 
+/* TWSX_BLOCK_STS register */
+#define TWSX_BLOCK_STS_RESET_PTR	BIT(0)
+
 /* Set BUS_MON_RST to reset bus monitor */
 #define BUS_MON_RST_MASK	BIT(3)
 
@@ -123,6 +133,7 @@ struct octeon_i2c {
 	void __iomem *twsi_base;
 	struct device *dev;
 	bool hlc_enabled;
+	bool block_enabled;
 	bool broken_irq_mode;
 	bool broken_irq_check;
 	void (*int_enable)(struct octeon_i2c *);
diff --git a/drivers/i2c/busses/i2c-thunderx-pcidrv.c b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
index 143d012fa43e..0dc08cd97e8a 100644
--- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c
+++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
@@ -168,6 +168,9 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
 	i2c->roff.twsi_int = 0x1010;
 	i2c->roff.sw_twsi_ext = 0x1018;
 	i2c->roff.mode = 0x1038;
+	i2c->roff.block_ctl = 0x1048;
+	i2c->roff.block_sts = 0x1050;
+	i2c->roff.block_fifo = 0x1058;
 
 	i2c->dev = dev;
 	pci_set_drvdata(pdev, i2c);
-- 
2.47.1

Re: [PATCH v13 1/3] i2c: octeon: fix return commenting

[Andy Shevchenko]

On Tue, Mar 18, 2025 at 4:16 AM Aryan Srivastava
<aryan.srivastava@alliedtelesis.co.nz> wrote:
>
> Kernel-docs require a ':' to signify the return behaviour of a function
> with within the comment. Many functions in this file were missing ':'
> after the "Returns" line, resulting in kernel-doc warnings.
>
> Add the ':' to satisfy kernel-doc requirements.

Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v13 2/3] i2c: octeon: remove 10-bit addressing support

[Andy Shevchenko]

On Tue, Mar 18, 2025 at 4:16 AM Aryan Srivastava
<aryan.srivastava@alliedtelesis.co.nz> wrote:
>
> The driver gives the illusion of 10-bit address support, but the upper
> 3 bits of the given address are always thrown away. Remove unnecessary
> considerations for 10 bit addressing and always complete 7 bit ops when
> using the hlc methods.

Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v13 2/3] i2c: octeon: remove 10-bit addressing support

[Andi Shyti]

Hi again,

On Tue, Mar 18, 2025 at 03:16:30PM +1300, Aryan Srivastava wrote:
> The driver gives the illusion of 10-bit address support, but the upper
> 3 bits of the given address are always thrown away. Remove unnecessary
> considerations for 10 bit addressing and always complete 7 bit ops when
> using the hlc methods.
> 
> Signed-off-by: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>

sorry, I commented on the previous version. Pasting it here to
keep the discussion on here.

"
Can someone from Marvell comment here?

The datasheet I have isn't very clear on this part. Also, I'd
like to know if there's any product line that could be negatively
impacted by this patch.
"

Thanks
Andi

Re: [PATCH v13 2/3] i2c: octeon: remove 10-bit addressing support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 688 bytes --]


> The datasheet I have isn't very clear on this part. Also, I'd
> like to know if there's any product line that could be negatively
> impacted by this patch.

In my whole I2C life, I have neither seen a target supporting 10-bit
addressing nor a a system that uses 10-bit addressing. I am even tempted
to remove support from the kernel omce in a while. If the support is
broken in this driver, it can be removed. A working version (if
possible) can be added again by someone who needs it. I am taking bets
it will be "never". Besides, the driver never set I2C_FUNC_10BIT_ADDR,
so it really shouldn't have been used anywhere.

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v13 2/3] i2c: octeon: remove 10-bit addressing support

[Andi Shyti]

On Wed, Mar 19, 2025 at 06:41:24AM +0100, Wolfram Sang wrote:
> > The datasheet I have isn't very clear on this part. Also, I'd
> > like to know if there's any product line that could be negatively
> > impacted by this patch.
> 
> In my whole I2C life, I have neither seen a target supporting 10-bit
> addressing nor a a system that uses 10-bit addressing.

mmhhh... I have to work with my memory and dig into my
documentations, but I think I've seen some STM sensors supporting
also 10 bit addresses.

> I am even tempted
> to remove support from the kernel omce in a while. If the support is
> broken in this driver, it can be removed.

The documentation I have is in line with the patch (I had to read
it twice, though), but I didn't want to exclude corner cases.

> A working version (if
> possible) can be added again by someone who needs it. I am taking bets
> it will be "never". Besides, the driver never set I2C_FUNC_10BIT_ADDR,
> so it really shouldn't have been used anywhere.
> 
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

OK, I had a little hesitation here, but if you are sure about it
(and Andy as well) I'll take it in.

Thanks, Wolfram!
Andi

Re: [PATCH v13 2/3] i2c: octeon: remove 10-bit addressing support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 263 bytes --]

Hi Andi,

> mmhhh... I have to work with my memory and dig into my
> documentations, but I think I've seen some STM sensors supporting
> also 10 bit addresses.

I'd be super-super-interested if you can remember which devices had
that!

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v13 3/3] i2c: octeon: add block-mode i2c operations

[Andi Shyti]

Hi Aryan,

few nitpicks between the lines. Please send only this patch as I
have applied already patch 1 and 2.

...

> +static int octeon_i2c_hlc_block_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs)
> +{
> +	int ret;
> +	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));
> +
> +	/* 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;

Do we need to disable the block mode?

> +	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) {

Please, do not declare the iterator inside the for loop.

> +		/* Byte-swap FIFO data and copy into msg buffer */
> +		__be64 rd = cpu_to_be64(__raw_readq(i2c->twsi_base + OCTEON_REG_BLOCK_FIFO(i2c)));
> +
> +		memcpy(&msgs[1].buf[i], &rd, min(8, msgs[1].len - i));
> +	}
> +
> +	octeon_i2c_block_disable(i2c);
> +	return ret;
> +}

...

>  #define OCTEON_REG_SW_TWSI(x)		((x)->roff.sw_twsi)
>  #define OCTEON_REG_TWSI_INT(x)		((x)->roff.twsi_int)
>  #define OCTEON_REG_SW_TWSI_EXT(x)	((x)->roff.sw_twsi_ext)
>  #define OCTEON_REG_MODE(x)		((x)->roff.mode)
> +#define OCTEON_REG_BLOCK_CTL(x)	(x->roff.block_ctl)
> +#define OCTEON_REG_BLOCK_STS(x)	(x->roff.block_sts)
> +#define OCTEON_REG_BLOCK_FIFO(x)	(x->roff.block_fifo)

Please use the ((x)->...) form.

Andi

>  
> -/* Set REFCLK_SRC and HS_MODE in TWSX_MODE register */
> +/* TWSX_MODE register */
>  #define TWSX_MODE_REFCLK_SRC	BIT(4)
> +#define TWSX_MODE_BLOCK_MODE	BIT(2)
>  #define TWSX_MODE_HS_MODE	BIT(0)
>  #define TWSX_MODE_HS_MASK	(TWSX_MODE_REFCLK_SRC | TWSX_MODE_HS_MODE)

Re: [PATCH v13 3/3] i2c: octeon: add block-mode i2c operations

[Aryan Srivastava]

Hi Andi,

On Wed, 2025-03-19 at 23:19 +0100, Andi Shyti wrote:
> Hi Aryan,
> 
> few nitpicks between the lines. Please send only this patch as I
> have applied already patch 1 and 2.
> 
> ...
> 
> > +static int octeon_i2c_hlc_block_comp_read(struct octeon_i2c *i2c,
> > struct i2c_msg *msgs)
> > +{
> > +       int ret;
> > +       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));
> > +
> > +       /* 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;
> 
> Do we need to disable the block mode?
> 
Do you mean, do we need to disable the block mode at all? i.e. have it
on all the time? Otherwise, it gets disabled at the bottom of this
func.
> > +       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) {
> 
> Please, do not declare the iterator inside the for loop.
> 
Done.
> > +               /* Byte-swap FIFO data and copy into msg buffer */
> > +               __be64 rd = cpu_to_be64(__raw_readq(i2c->twsi_base
> > + OCTEON_REG_BLOCK_FIFO(i2c)));
> > +
> > +               memcpy(&msgs[1].buf[i], &rd, min(8, msgs[1].len -
> > i));
> > +       }
> > +
> > +       octeon_i2c_block_disable(i2c);
> > +       return ret;
> > +}
> 
> ...
> 
> >  #define OCTEON_REG_SW_TWSI(x)          ((x)->roff.sw_twsi)
> >  #define OCTEON_REG_TWSI_INT(x)         ((x)->roff.twsi_int)
> >  #define OCTEON_REG_SW_TWSI_EXT(x)      ((x)->roff.sw_twsi_ext)
> >  #define OCTEON_REG_MODE(x)             ((x)->roff.mode)
> > +#define OCTEON_REG_BLOCK_CTL(x)        (x->roff.block_ctl)
> > +#define OCTEON_REG_BLOCK_STS(x)        (x->roff.block_sts)
> > +#define OCTEON_REG_BLOCK_FIFO(x)       (x->roff.block_fifo)
> 
> Please use the ((x)->...) form.
> 
Done.
> Andi
> 
> >  
> > -/* Set REFCLK_SRC and HS_MODE in TWSX_MODE register */
> > +/* TWSX_MODE register */
> >  #define TWSX_MODE_REFCLK_SRC   BIT(4)
> > +#define TWSX_MODE_BLOCK_MODE   BIT(2)
> >  #define TWSX_MODE_HS_MODE      BIT(0)
> >  #define TWSX_MODE_HS_MASK      (TWSX_MODE_REFCLK_SRC |
> > TWSX_MODE_HS_MODE)

Re: [PATCH v13 3/3] i2c: octeon: add block-mode i2c operations

[Andi Shyti]

Hi Aryan,

On Thu, Mar 20, 2025 at 01:36:12AM +0000, Aryan Srivastava wrote:
> On Wed, 2025-03-19 at 23:19 +0100, Andi Shyti wrote:
> > > +static int octeon_i2c_hlc_block_comp_read(struct octeon_i2c *i2c,
> > > struct i2c_msg *msgs)
> > > +{
> > > +       int ret;
> > > +       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));
> > > +
> > > +       /* 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;
> > 
> > Do we need to disable the block mode?
> > 
> Do you mean, do we need to disable the block mode at all? i.e. have it
> on all the time? Otherwise, it gets disabled at the bottom of this
> func.

yes, but you return earlier, right?

Thanks,
Andi

...

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

...

Re: [PATCH v13 3/3] i2c: octeon: add block-mode i2c operations

[Aryan Srivastava]

On Thu, 2025-03-20 at 10:01 +0100, Andi Shyti wrote:
> Hi Aryan,
> 
> On Thu, Mar 20, 2025 at 01:36:12AM +0000, Aryan Srivastava wrote:
> > On Wed, 2025-03-19 at 23:19 +0100, Andi Shyti wrote:
> > > > +static int octeon_i2c_hlc_block_comp_read(struct octeon_i2c
> > > > *i2c,
> > > > struct i2c_msg *msgs)
> > > > +{
> > > > +       int ret;
> > > > +       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));
> > > > +
> > > > +       /* 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;
> > > 
> > > Do we need to disable the block mode?
> > > 
> > Do you mean, do we need to disable the block mode at all? i.e. have
> > it
> > on all the time? Otherwise, it gets disabled at the bottom of this
> > func.
> 
> yes, but you return earlier, right?
> 
Ah yes, good catch. I will fix this up, thank you :)

Cheers, Aryan.