2023-07-06 00:05:30

by Aryan Srivastava

[permalink] [raw]
Subject: [PATCH] i2c:octeon:Add block-mode r/w

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.

Signed-off-by: Aryan Srivastava <[email protected]>
---
drivers/i2c/busses/i2c-octeon-core.c | 162 ++++++++++++++++++++++-
drivers/i2c/busses/i2c-octeon-core.h | 14 ++
drivers/i2c/busses/i2c-thunderx-pcidrv.c | 4 +
3 files changed, 174 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index 845eda70b8ca..304db70cb1e0 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -130,6 +130,24 @@ 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)
+{
+ if (i2c->block_enabled || !TWSI_BLOCK_CTL(i2c))
+ return;
+
+ i2c->block_enabled = true;
+ octeon_i2c_writeq_flush(
+ TWSI_MODE_STRETCH | TWSI_MODE_BLOCK_MODE, i2c->twsi_base + TWSI_MODE(i2c));
+}
+
+static void octeon_i2c_block_disable(struct octeon_i2c *i2c)
+{
+ if (!i2c->block_enabled || !TWSI_BLOCK_CTL(i2c))
+ return;
+
+ i2c->block_enabled = false;
+ octeon_i2c_writeq_flush(TWSI_MODE_STRETCH, i2c->twsi_base + TWSI_MODE(i2c));
+}
/**
* octeon_i2c_hlc_wait - wait for an HLC operation to complete
* @i2c: The struct octeon_i2c
@@ -268,6 +286,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);
@@ -594,6 +613,128 @@ static int octeon_i2c_hlc_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msg
return ret;
}

+/* high-level-controller composite block write+read, m[0]len<=2, m[1]len<=1024 */
+static int octeon_i2c_hlc_block_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs)
+{
+ int i, j, len, ret = 0;
+ u64 cmd, rd;
+
+ octeon_i2c_hlc_enable(i2c);
+ octeon_i2c_block_enable(i2c);
+
+ len = msgs[1].len - 1;
+ /* Prepare core command */
+ cmd = SW_TWSI_V | SW_TWSI_R | SW_TWSI_SOVR;
+ /* SIZE */
+ octeon_i2c_writeq_flush((u64)(len), i2c->twsi_base + TWSI_BLOCK_CTL(i2c));
+ /* A */
+ cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT;
+
+ if (msgs[0].flags & I2C_M_TEN)
+ cmd |= SW_TWSI_OP_10_IA;
+ else
+ cmd |= SW_TWSI_OP_7_IA;
+
+ if (msgs[0].len == 2) {
+ u64 ext = 0;
+
+ cmd |= SW_TWSI_EIA;
+ ext = (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
+ cmd |= (u64)msgs[0].buf[1] << SW_TWSI_IA_SHIFT;
+ octeon_i2c_writeq_flush(ext, i2c->twsi_base + SW_TWSI_EXT(i2c));
+ } else {
+ cmd |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
+ }
+
+ /* Send command to core (send data to FIFO) */
+ octeon_i2c_hlc_int_clear(i2c);
+ octeon_i2c_writeq_flush(cmd, i2c->twsi_base + SW_TWSI(i2c));
+
+ /* Wait for transaction to complete */
+ ret = octeon_i2c_hlc_wait(i2c);
+ if (ret)
+ goto err;
+
+ cmd = __raw_readq(i2c->twsi_base + SW_TWSI(i2c));
+ if ((cmd & SW_TWSI_R) == 0)
+ return octeon_i2c_check_status(i2c, false);
+
+ /* read data in FIFO */
+ octeon_i2c_writeq_flush(TWSI_BLOCK_STS_RESET_PTR, i2c->twsi_base + TWSI_BLOCK_STS(i2c));
+ for (i = 0; i < len; i += 8) {
+ rd = __raw_readq(i2c->twsi_base + TWSI_BLOCK_FIFO(i2c));
+ for (j = 7; j >= 0; j--)
+ msgs[1].buf[i + (7 - j)] = (rd >> (8 * j)) & 0xff;
+ }
+
+ octeon_i2c_block_disable(i2c);
+
+err:
+ return ret;
+}
+
+/* high-level-controller composite block write+write, m[0]len<=2, m[1]len<=1024 */
+static int octeon_i2c_hlc_block_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msgs)
+{
+ bool set_ext = false;
+ int i, j, len, ret = 0;
+ u64 cmd, buf = 0, ext = 0;
+
+ octeon_i2c_hlc_enable(i2c);
+ octeon_i2c_block_enable(i2c);
+
+ len = msgs[1].len - 1;
+ /* Prepare core command */
+ cmd = SW_TWSI_V | SW_TWSI_SOVR;
+ /* SIZE */
+ octeon_i2c_writeq_flush((u64)(len), i2c->twsi_base + TWSI_BLOCK_CTL(i2c));
+ /* A */
+ cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT;
+
+ if (msgs[0].flags & I2C_M_TEN)
+ cmd |= SW_TWSI_OP_10_IA;
+ else
+ cmd |= SW_TWSI_OP_7_IA;
+
+ if (msgs[0].len == 2) {
+ cmd |= SW_TWSI_EIA;
+ ext |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
+ set_ext = true;
+ cmd |= (u64)msgs[0].buf[1] << SW_TWSI_IA_SHIFT;
+ } else {
+ cmd |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
+ }
+
+ /* Write msg into FIFO buffer */
+ octeon_i2c_writeq_flush(TWSI_BLOCK_STS_RESET_PTR, i2c->twsi_base + TWSI_BLOCK_STS(i2c));
+ for (i = 0; i < len; i += 8) {
+ buf = 0;
+ for (j = 7; j >= 0; j--)
+ buf |= (msgs[1].buf[i + (7 - j)] << (8 * j));
+ 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) */
+ octeon_i2c_hlc_int_clear(i2c);
+ octeon_i2c_writeq_flush(cmd, i2c->twsi_base + SW_TWSI(i2c));
+
+ /* Wait for transaction to complete */
+ ret = octeon_i2c_hlc_wait(i2c);
+ if (ret)
+ goto err;
+
+ 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);
+
+err:
+ return ret;
+}
+
/**
* octeon_i2c_xfer - The driver's master_xfer function
* @adap: Pointer to the i2c_adapter structure
@@ -619,13 +760,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 && TWSI_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;
+ }
}
}

@@ -720,6 +869,7 @@ int octeon_i2c_init_lowlevel(struct octeon_i2c *i2c)
/* toggle twice to force both teardowns */
octeon_i2c_hlc_enable(i2c);
octeon_i2c_hlc_disable(i2c);
+
return 0;
}

diff --git a/drivers/i2c/busses/i2c-octeon-core.h b/drivers/i2c/busses/i2c-octeon-core.h
index 9bb9f64fdda0..b889ecf9968d 100644
--- a/drivers/i2c/busses/i2c-octeon-core.h
+++ b/drivers/i2c/busses/i2c-octeon-core.h
@@ -85,6 +85,11 @@
#define TWSI_INT_SDA BIT_ULL(10)
#define TWSI_INT_SCL BIT_ULL(11)

+#define TWSI_MODE_STRETCH BIT_ULL(1)
+#define TWSI_MODE_BLOCK_MODE BIT_ULL(2)
+
+#define TWSI_BLOCK_STS_RESET_PTR BIT_ULL(0)
+#define TWSI_BLOCK_STS_BUSY BIT_ULL(1)
#define I2C_OCTEON_EVENT_WAIT 80 /* microseconds */

/* Register offsets */
@@ -92,11 +97,19 @@ struct octeon_i2c_reg_offset {
unsigned int sw_twsi;
unsigned int twsi_int;
unsigned int sw_twsi_ext;
+ unsigned int twsi_mode;
+ unsigned int twsi_block_ctl;
+ unsigned int twsi_block_sts;
+ unsigned int twsi_block_fifo;
};

#define SW_TWSI(x) (x->roff.sw_twsi)
#define TWSI_INT(x) (x->roff.twsi_int)
#define SW_TWSI_EXT(x) (x->roff.sw_twsi_ext)
+#define TWSI_MODE(x) (x->roff.twsi_mode)
+#define TWSI_BLOCK_CTL(x) (x->roff.twsi_block_ctl)
+#define TWSI_BLOCK_STS(x) (x->roff.twsi_block_sts)
+#define TWSI_BLOCK_FIFO(x) (x->roff.twsi_block_fifo)

struct octeon_i2c {
wait_queue_head_t queue;
@@ -110,6 +123,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 a77cd86fe75e..abde98117d7e 100644
--- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c
+++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
@@ -165,6 +165,10 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
i2c->roff.sw_twsi = 0x1000;
i2c->roff.twsi_int = 0x1010;
i2c->roff.sw_twsi_ext = 0x1018;
+ i2c->roff.twsi_mode = 0x1038;
+ i2c->roff.twsi_block_ctl = 0x1048;
+ i2c->roff.twsi_block_sts = 0x1050;
+ i2c->roff.twsi_block_fifo = 0x1058;

i2c->dev = dev;
pci_set_drvdata(pdev, i2c);
--
2.41.0



2023-07-25 23:59:54

by Andi Shyti

[permalink] [raw]
Subject: Re: [PATCH] i2c:octeon:Add block-mode r/w

Hi Aryan,

On Thu, Jul 06, 2023 at 11:45:00AM +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.
>
> Signed-off-by: Aryan Srivastava <[email protected]>

thanks for your patch, could you please run checkpatch.pl,
clean it up a little (e.g. comments like /* A */) and resend it?

Thanks,
Andi

2023-09-03 13:18:56

by Andi Shyti

[permalink] [raw]
Subject: Re: [PATCH] i2c:octeon:Add block-mode r/w

Hi Aryan,

[...]

> I have re-ran the patch through checkpatch --strict and changed the
> comments

Thanks!

[...]

> +static void octeon_i2c_block_disable(struct octeon_i2c *i2c)
> +{
> + if (!i2c->block_enabled || !TWSI_BLOCK_CTL(i2c))
> + return;
> +
> + i2c->block_enabled = false;
> + octeon_i2c_writeq_flush(TWSI_MODE_STRETCH, i2c->twsi_base + TWSI_MODE(i2c));
> +}

can you leave a blank line here?

> /**
> * octeon_i2c_hlc_wait - wait for an HLC operation to complete
> * @i2c: The struct octeon_i2c
> @@ -268,6 +286,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);
> @@ -594,6 +613,128 @@ static int octeon_i2c_hlc_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msg
> return ret;
> }
>
> +/* high-level-controller composite block write+read, m[0]len<=2, m[1]len<=1024 */

can you please improve the comment?

> +static int octeon_i2c_hlc_block_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs)
> +{
> + int i, j, len, ret = 0;
> + u64 cmd, rd;
> +
> + octeon_i2c_hlc_enable(i2c);
> + octeon_i2c_block_enable(i2c);
> +
> + len = msgs[1].len - 1;

why -1?

> + /* Prepare core command */
> + cmd = SW_TWSI_V | SW_TWSI_R | SW_TWSI_SOVR;

cmd needs to be initialized to '0'.

> + /* SIZE */
> + octeon_i2c_writeq_flush((u64)(len), i2c->twsi_base + TWSI_BLOCK_CTL(i2c));
> + /* ADDR */
> + cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT;

can you please write some more meaningful comments?

[...]

> + /* Wait for transaction to complete */
> + ret = octeon_i2c_hlc_wait(i2c);
> + if (ret)
> + goto err;

just return err, no point having a goto label here.

[...]

> +err:
> + return ret;
> +}
> +
> +/* high-level-controller composite block write+write, m[0]len<=2, m[1]len<=1024 */
> +static int octeon_i2c_hlc_block_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msgs)

more or less same comments apply here as _comp_read()

[...]

> @@ -720,6 +869,7 @@ int octeon_i2c_init_lowlevel(struct octeon_i2c *i2c)
> /* toggle twice to force both teardowns */
> octeon_i2c_hlc_enable(i2c);
> octeon_i2c_hlc_disable(i2c);
> +

this change does not belong here

> return 0;
> }
>

[...]

> #define TWSI_INT_SDA BIT_ULL(10)
> #define TWSI_INT_SCL BIT_ULL(11)
>
> +#define TWSI_MODE_STRETCH BIT_ULL(1)
> +#define TWSI_MODE_BLOCK_MODE BIT_ULL(2)
> +
> +#define TWSI_BLOCK_STS_RESET_PTR BIT_ULL(0)
> +#define TWSI_BLOCK_STS_BUSY BIT_ULL(1)

The alignment here is a bit off.

Andi

> #define I2C_OCTEON_EVENT_WAIT 80 /* microseconds */

2023-09-05 16:05:39

by Aryan Srivastava

[permalink] [raw]
Subject: [PATCH] i2c:octeon:Add block-mode r/w

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.

Signed-off-by: Aryan Srivastava <[email protected]>
---
Changes in v2:
- comment style and formatting.

Changes in v3:
- comment style and formatting.
---
drivers/i2c/busses/i2c-octeon-core.c | 158 ++++++++++++++++++++++-
drivers/i2c/busses/i2c-octeon-core.h | 14 ++
drivers/i2c/busses/i2c-thunderx-pcidrv.c | 4 +
3 files changed, 170 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index 845eda70b8ca..940cc4647cb0 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -130,6 +130,25 @@ 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)
+{
+ if (i2c->block_enabled || !TWSI_BLOCK_CTL(i2c))
+ return;
+
+ i2c->block_enabled = true;
+ octeon_i2c_writeq_flush(TWSI_MODE_STRETCH
+ | TWSI_MODE_BLOCK_MODE, i2c->twsi_base + TWSI_MODE(i2c));
+}
+
+static void octeon_i2c_block_disable(struct octeon_i2c *i2c)
+{
+ if (!i2c->block_enabled || !TWSI_BLOCK_CTL(i2c))
+ return;
+
+ i2c->block_enabled = false;
+ octeon_i2c_writeq_flush(TWSI_MODE_STRETCH, i2c->twsi_base + TWSI_MODE(i2c));
+}
+
/**
* octeon_i2c_hlc_wait - wait for an HLC operation to complete
* @i2c: The struct octeon_i2c
@@ -268,6 +287,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);
@@ -594,6 +614,124 @@ 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 */
+static int octeon_i2c_hlc_block_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs)
+{
+ int i, j, len, ret = 0;
+ u64 cmd = 0, rd = 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 + TWSI_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;
+
+ if (msgs[0].flags & I2C_M_TEN)
+ cmd |= SW_TWSI_OP_10_IA;
+ else
+ cmd |= SW_TWSI_OP_7_IA;
+
+ if (msgs[0].len == 2) {
+ u64 ext = 0;
+
+ cmd |= SW_TWSI_EIA;
+ ext = (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
+ cmd |= (u64)msgs[0].buf[1] << SW_TWSI_IA_SHIFT;
+ octeon_i2c_writeq_flush(ext, i2c->twsi_base + SW_TWSI_EXT(i2c));
+ } else {
+ cmd |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
+ }
+
+ /* Send command to core (send data to FIFO) */
+ octeon_i2c_hlc_int_clear(i2c);
+ octeon_i2c_writeq_flush(cmd, i2c->twsi_base + SW_TWSI(i2c));
+
+ /* Wait for transaction to complete */
+ ret = octeon_i2c_hlc_wait(i2c);
+ if (ret)
+ return ret;
+
+ cmd = __raw_readq(i2c->twsi_base + SW_TWSI(i2c));
+ if ((cmd & SW_TWSI_R) == 0)
+ return octeon_i2c_check_status(i2c, false);
+
+ /* read data in FIFO */
+ octeon_i2c_writeq_flush(TWSI_BLOCK_STS_RESET_PTR, i2c->twsi_base + TWSI_BLOCK_STS(i2c));
+ for (i = 0; i < len; i += 8) {
+ rd = __raw_readq(i2c->twsi_base + TWSI_BLOCK_FIFO(i2c));
+ for (j = 7; j >= 0; j--)
+ msgs[1].buf[i + (7 - j)] = (rd >> (8 * j)) & 0xff;
+ }
+
+ octeon_i2c_block_disable(i2c);
+ return ret;
+}
+
+/* high-level-controller composite block write+write, m[0]len<=2, m[1]len<=1024 */
+static int octeon_i2c_hlc_block_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msgs)
+{
+ bool set_ext = false;
+ int i, j, len, ret = 0;
+ u64 cmd, buf = 0, 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 + TWSI_BLOCK_CTL(i2c));
+
+ /* Prepare core command */
+ cmd = SW_TWSI_V | SW_TWSI_SOVR;
+ cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT;
+
+ if (msgs[0].flags & I2C_M_TEN)
+ cmd |= SW_TWSI_OP_10_IA;
+ else
+ cmd |= SW_TWSI_OP_7_IA;
+
+ if (msgs[0].len == 2) {
+ cmd |= SW_TWSI_EIA;
+ ext |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
+ set_ext = true;
+ cmd |= (u64)msgs[0].buf[1] << SW_TWSI_IA_SHIFT;
+ } else {
+ cmd |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
+ }
+
+ /* Write msg into FIFO buffer */
+ octeon_i2c_writeq_flush(TWSI_BLOCK_STS_RESET_PTR, i2c->twsi_base + TWSI_BLOCK_STS(i2c));
+ for (i = 0; i < len; i += 8) {
+ buf = 0;
+ for (j = 7; j >= 0; j--)
+ buf |= (msgs[1].buf[i + (7 - j)] << (8 * j));
+ 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) */
+ octeon_i2c_hlc_int_clear(i2c);
+ octeon_i2c_writeq_flush(cmd, i2c->twsi_base + SW_TWSI(i2c));
+
+ /* Wait for transaction to complete */
+ ret = octeon_i2c_hlc_wait(i2c);
+ if (ret)
+ goto ret;
+
+ 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;
+}
+
/**
* octeon_i2c_xfer - The driver's master_xfer function
* @adap: Pointer to the i2c_adapter structure
@@ -619,13 +757,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 && TWSI_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 9bb9f64fdda0..81fcf413c890 100644
--- a/drivers/i2c/busses/i2c-octeon-core.h
+++ b/drivers/i2c/busses/i2c-octeon-core.h
@@ -85,6 +85,11 @@
#define TWSI_INT_SDA BIT_ULL(10)
#define TWSI_INT_SCL BIT_ULL(11)

+#define TWSI_MODE_STRETCH BIT_ULL(1)
+#define TWSI_MODE_BLOCK_MODE BIT_ULL(2)
+
+#define TWSI_BLOCK_STS_RESET_PTR BIT_ULL(0)
+#define TWSI_BLOCK_STS_BUSY BIT_ULL(1)
#define I2C_OCTEON_EVENT_WAIT 80 /* microseconds */

/* Register offsets */
@@ -92,11 +97,19 @@ struct octeon_i2c_reg_offset {
unsigned int sw_twsi;
unsigned int twsi_int;
unsigned int sw_twsi_ext;
+ unsigned int twsi_mode;
+ unsigned int twsi_block_ctl;
+ unsigned int twsi_block_sts;
+ unsigned int twsi_block_fifo;
};

#define SW_TWSI(x) (x->roff.sw_twsi)
#define TWSI_INT(x) (x->roff.twsi_int)
#define SW_TWSI_EXT(x) (x->roff.sw_twsi_ext)
+#define TWSI_MODE(x) (x->roff.twsi_mode)
+#define TWSI_BLOCK_CTL(x) (x->roff.twsi_block_ctl)
+#define TWSI_BLOCK_STS(x) (x->roff.twsi_block_sts)
+#define TWSI_BLOCK_FIFO(x) (x->roff.twsi_block_fifo)

struct octeon_i2c {
wait_queue_head_t queue;
@@ -110,6 +123,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 a77cd86fe75e..abde98117d7e 100644
--- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c
+++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
@@ -165,6 +165,10 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
i2c->roff.sw_twsi = 0x1000;
i2c->roff.twsi_int = 0x1010;
i2c->roff.sw_twsi_ext = 0x1018;
+ i2c->roff.twsi_mode = 0x1038;
+ i2c->roff.twsi_block_ctl = 0x1048;
+ i2c->roff.twsi_block_sts = 0x1050;
+ i2c->roff.twsi_block_fifo = 0x1058;

i2c->dev = dev;
pci_set_drvdata(pdev, i2c);
--
2.42.0

2023-09-05 18:47:45

by Andi Shyti

[permalink] [raw]
Subject: Re: [PATCH] i2c:octeon:Add block-mode r/w

Hi Aryan,

In the title, please leave a space after the ':'

i2c: octeon: Add block-mode r/w

Please check with "git log drivers..." to see what's the rule in
a particular community.

I guess Wolfram can fix this, though, before pushing.

[...]

> +/* high-level-controller composite block write+read, msg0=addr, msg1=data */

I think this comment is fine and great to have it, but it's
missing a bit of clarity, can you please expand the concept?

> +static int octeon_i2c_hlc_block_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs)
> +{
> + int i, j, len, ret = 0;
> + u64 cmd = 0, rd = 0;

can please you move rd, j inside the for loop? The basic common
sense is to have all variable declared in the innermost section
in order to avoid confusion.

It's a nitpick though, not a strong comment and, afaik, not a
real rule.

Same comment for the function below.

> +
> + 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 + TWSI_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;
> +
> + if (msgs[0].flags & I2C_M_TEN)
> + cmd |= SW_TWSI_OP_10_IA;
> + else
> + cmd |= SW_TWSI_OP_7_IA;
> +
> + if (msgs[0].len == 2) {
> + u64 ext = 0;
> +
> + cmd |= SW_TWSI_EIA;
> + ext = (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
> + cmd |= (u64)msgs[0].buf[1] << SW_TWSI_IA_SHIFT;
> + octeon_i2c_writeq_flush(ext, i2c->twsi_base + SW_TWSI_EXT(i2c));
> + } else {
> + cmd |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
> + }

This first part is basically a copy/paste with the write()
function... can we put them together in a common function?

Can we put as much as we can in a single function?

> + /* Send command to core (send data to FIFO) */
> + octeon_i2c_hlc_int_clear(i2c);
> + octeon_i2c_writeq_flush(cmd, i2c->twsi_base + SW_TWSI(i2c));
> +
> + /* Wait for transaction to complete */
> + ret = octeon_i2c_hlc_wait(i2c);
> + if (ret)
> + return ret;
> +
> + cmd = __raw_readq(i2c->twsi_base + SW_TWSI(i2c));
> + if ((cmd & SW_TWSI_R) == 0)
> + return octeon_i2c_check_status(i2c, false);
> +
> + /* read data in FIFO */
> + octeon_i2c_writeq_flush(TWSI_BLOCK_STS_RESET_PTR, i2c->twsi_base + TWSI_BLOCK_STS(i2c));
> + for (i = 0; i < len; i += 8) {
> + rd = __raw_readq(i2c->twsi_base + TWSI_BLOCK_FIFO(i2c));
> + for (j = 7; j >= 0; j--)

is len always a multiple of 8?

> + msgs[1].buf[i + (7 - j)] = (rd >> (8 * j)) & 0xff;
> + }
> +
> + octeon_i2c_block_disable(i2c);
> + return ret;
> +}

[...]

> - 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 && TWSI_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;
> + }

the rest looks good...

Thanks,
Andi

2023-09-13 02:38:19

by Aryan Srivastava

[permalink] [raw]
Subject: [PATCH] THUNDERX_I2C_BLOCK_MODE

i2c:octeon:Add block-mode r/w

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.

Signed-off-by: Aryan Srivastava <[email protected]>
---
drivers/i2c/busses/i2c-octeon-core.c | 198 +++++++++++++++++++----
drivers/i2c/busses/i2c-octeon-core.h | 14 ++
drivers/i2c/busses/i2c-thunderx-pcidrv.c | 4 +
3 files changed, 186 insertions(+), 30 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index 845eda70b8ca..4c704262d228 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -130,6 +130,25 @@ 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)
+{
+ if (i2c->block_enabled || !TWSI_BLOCK_CTL(i2c))
+ return;
+
+ i2c->block_enabled = true;
+ octeon_i2c_writeq_flush(TWSI_MODE_STRETCH
+ | TWSI_MODE_BLOCK_MODE, i2c->twsi_base + TWSI_MODE(i2c));
+}
+
+static void octeon_i2c_block_disable(struct octeon_i2c *i2c)
+{
+ if (!i2c->block_enabled || !TWSI_BLOCK_CTL(i2c))
+ return;
+
+ i2c->block_enabled = false;
+ octeon_i2c_writeq_flush(TWSI_MODE_STRETCH, i2c->twsi_base + TWSI_MODE(i2c));
+}
+
/**
* octeon_i2c_hlc_wait - wait for an HLC operation to complete
* @i2c: The struct octeon_i2c
@@ -268,6 +287,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);
@@ -485,6 +505,37 @@ static int octeon_i2c_hlc_write(struct octeon_i2c *i2c, struct i2c_msg *msgs)
return ret;
}

+/* Process hlc transaction */
+static int octeon_i2c_hlc_cmd_send(struct octeon_i2c *i2c, u64 cmd)
+{
+ octeon_i2c_hlc_int_clear(i2c);
+ octeon_i2c_writeq_flush(cmd, i2c->twsi_base + SW_TWSI(i2c));
+
+ return octeon_i2c_hlc_wait(i2c);
+}
+
+/* Construct and send i2c transaction core cmd */
+static int octeon_i2c_hlc_cmd(struct octeon_i2c *i2c, struct i2c_msg msg, u64 cmd)
+{
+ if (msg.flags & I2C_M_TEN)
+ cmd |= SW_TWSI_OP_10_IA;
+ else
+ cmd |= SW_TWSI_OP_7_IA;
+
+ if (msg.len == 2) {
+ u64 ext = 0;
+
+ cmd |= SW_TWSI_EIA;
+ ext = (u64)msg.buf[0] << SW_TWSI_IA_SHIFT;
+ cmd |= (u64)msg.buf[1] << SW_TWSI_IA_SHIFT;
+ octeon_i2c_writeq_flush(ext, i2c->twsi_base + SW_TWSI_EXT(i2c));
+ } else {
+ cmd |= (u64)msg.buf[0] << SW_TWSI_IA_SHIFT;
+ }
+
+ return octeon_i2c_hlc_cmd_send(i2c, cmd);
+}
+
/* high-level-controller composite write+read, msg0=addr, msg1=data */
static int octeon_i2c_hlc_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs)
{
@@ -499,26 +550,8 @@ static int octeon_i2c_hlc_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs
/* A */
cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT;

- if (msgs[0].flags & I2C_M_TEN)
- cmd |= SW_TWSI_OP_10_IA;
- else
- cmd |= SW_TWSI_OP_7_IA;
-
- if (msgs[0].len == 2) {
- u64 ext = 0;
-
- cmd |= SW_TWSI_EIA;
- ext = (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
- cmd |= (u64)msgs[0].buf[1] << SW_TWSI_IA_SHIFT;
- octeon_i2c_writeq_flush(ext, i2c->twsi_base + SW_TWSI_EXT(i2c));
- } else {
- cmd |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
- }
-
- octeon_i2c_hlc_int_clear(i2c);
- octeon_i2c_writeq_flush(cmd, i2c->twsi_base + SW_TWSI(i2c));
-
- ret = octeon_i2c_hlc_wait(i2c);
+ /* Send core command */
+ ret = octeon_i2c_hlc_cmd(i2c, msgs[0], cmd);
if (ret)
goto err;

@@ -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;

@@ -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
+ * Used in the case where the i2c xfer is for greater than 8 bytes of read data.
+ */
+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 + TWSI_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;
+
+ /* Send core command */
+ ret = octeon_i2c_hlc_cmd(i2c, msgs[0], cmd);
+ if (ret)
+ return ret;
+
+ cmd = __raw_readq(i2c->twsi_base + SW_TWSI(i2c));
+ if ((cmd & SW_TWSI_R) == 0)
+ return octeon_i2c_check_status(i2c, false);
+
+ /* 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;
+ }
+
+ octeon_i2c_block_disable(i2c);
+ return ret;
+}
+
+/**
+ * high-level-controller composite block write+write, m[0]len<=2, m[1]len<=1024
+ * Used in the case where the i2c xfer is for greater than 8 bytes of write data.
+ */
+static int octeon_i2c_hlc_block_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msgs)
+{
+ bool set_ext = false;
+ int i, j, 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 + TWSI_BLOCK_CTL(i2c));
+
+ /* Prepare core command */
+ cmd = SW_TWSI_V | SW_TWSI_SOVR;
+ cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT;
+
+ if (msgs[0].flags & I2C_M_TEN)
+ cmd |= SW_TWSI_OP_10_IA;
+ else
+ cmd |= SW_TWSI_OP_7_IA;
+
+ if (msgs[0].len == 2) {
+ cmd |= SW_TWSI_EIA;
+ ext |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
+ set_ext = true;
+ cmd |= (u64)msgs[0].buf[1] << SW_TWSI_IA_SHIFT;
+ } else {
+ cmd |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
+ }
+
+ /* Write msg into FIFO buffer */
+ octeon_i2c_writeq_flush(TWSI_BLOCK_STS_RESET_PTR, i2c->twsi_base + TWSI_BLOCK_STS(i2c));
+ for (i = 0; i < len; i += 8) {
+ u64 buf = 0;
+ for (j = 7; j >= 0; j--)
+ buf |= (msgs[1].buf[i + (7 - j)] << (8 * j));
+ 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;
+
+ 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;
+}
+
/**
* octeon_i2c_xfer - The driver's master_xfer function
* @adap: Pointer to the i2c_adapter structure
@@ -619,13 +749,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 && TWSI_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 9bb9f64fdda0..81fcf413c890 100644
--- a/drivers/i2c/busses/i2c-octeon-core.h
+++ b/drivers/i2c/busses/i2c-octeon-core.h
@@ -85,6 +85,11 @@
#define TWSI_INT_SDA BIT_ULL(10)
#define TWSI_INT_SCL BIT_ULL(11)

+#define TWSI_MODE_STRETCH BIT_ULL(1)
+#define TWSI_MODE_BLOCK_MODE BIT_ULL(2)
+
+#define TWSI_BLOCK_STS_RESET_PTR BIT_ULL(0)
+#define TWSI_BLOCK_STS_BUSY BIT_ULL(1)
#define I2C_OCTEON_EVENT_WAIT 80 /* microseconds */

/* Register offsets */
@@ -92,11 +97,19 @@ struct octeon_i2c_reg_offset {
unsigned int sw_twsi;
unsigned int twsi_int;
unsigned int sw_twsi_ext;
+ unsigned int twsi_mode;
+ unsigned int twsi_block_ctl;
+ unsigned int twsi_block_sts;
+ unsigned int twsi_block_fifo;
};

#define SW_TWSI(x) (x->roff.sw_twsi)
#define TWSI_INT(x) (x->roff.twsi_int)
#define SW_TWSI_EXT(x) (x->roff.sw_twsi_ext)
+#define TWSI_MODE(x) (x->roff.twsi_mode)
+#define TWSI_BLOCK_CTL(x) (x->roff.twsi_block_ctl)
+#define TWSI_BLOCK_STS(x) (x->roff.twsi_block_sts)
+#define TWSI_BLOCK_FIFO(x) (x->roff.twsi_block_fifo)

struct octeon_i2c {
wait_queue_head_t queue;
@@ -110,6 +123,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 a77cd86fe75e..abde98117d7e 100644
--- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c
+++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
@@ -165,6 +165,10 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
i2c->roff.sw_twsi = 0x1000;
i2c->roff.twsi_int = 0x1010;
i2c->roff.sw_twsi_ext = 0x1018;
+ i2c->roff.twsi_mode = 0x1038;
+ i2c->roff.twsi_block_ctl = 0x1048;
+ i2c->roff.twsi_block_sts = 0x1050;
+ i2c->roff.twsi_block_fifo = 0x1058;

i2c->dev = dev;
pci_set_drvdata(pdev, i2c);
--
2.42.0

2023-10-25 21:20:49

by Aryan Srivastava

[permalink] [raw]
Subject: Re: [PATCH] i2c:octeon:Add block-mode r/w

Hi Andi,

Did you have any more comments on my patch?

Thank you,
Aryan.

2024-04-15 00:52:42

by Aryan Srivastava

[permalink] [raw]
Subject: [PATCH v4] i2c: octeon: Add block-mode r/w

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.

Signed-off-by: Aryan Srivastava <[email protected]>
---
Changes in v2:
- comment style and formatting.

Changes in v3:
- comment style and formatting.

Changes in v4:
- Refactoring common code.
- Additional comments.
---
drivers/i2c/busses/i2c-octeon-core.c | 196 +++++++++++++++++++----
drivers/i2c/busses/i2c-octeon-core.h | 14 ++
drivers/i2c/busses/i2c-thunderx-pcidrv.c | 4 +
3 files changed, 185 insertions(+), 29 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index 845eda70b8ca..1ba4ac24b02a 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -130,6 +130,25 @@ 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)
+{
+ if (i2c->block_enabled || !TWSI_BLOCK_CTL(i2c))
+ return;
+
+ i2c->block_enabled = true;
+ octeon_i2c_writeq_flush(TWSI_MODE_STRETCH
+ | TWSI_MODE_BLOCK_MODE, i2c->twsi_base + TWSI_MODE(i2c));
+}
+
+static void octeon_i2c_block_disable(struct octeon_i2c *i2c)
+{
+ if (!i2c->block_enabled || !TWSI_BLOCK_CTL(i2c))
+ return;
+
+ i2c->block_enabled = false;
+ octeon_i2c_writeq_flush(TWSI_MODE_STRETCH, i2c->twsi_base + TWSI_MODE(i2c));
+}
+
/**
* octeon_i2c_hlc_wait - wait for an HLC operation to complete
* @i2c: The struct octeon_i2c
@@ -268,6 +287,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);
@@ -485,40 +505,53 @@ static int octeon_i2c_hlc_write(struct octeon_i2c *i2c, struct i2c_msg *msgs)
return ret;
}

-/* high-level-controller composite write+read, msg0=addr, msg1=data */
-static int octeon_i2c_hlc_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs)
+/* Process hlc transaction */
+static int octeon_i2c_hlc_cmd_send(struct octeon_i2c *i2c, u64 cmd)
{
- int i, j, ret = 0;
- u64 cmd;
-
- octeon_i2c_hlc_enable(i2c);
+ octeon_i2c_hlc_int_clear(i2c);
+ octeon_i2c_writeq_flush(cmd, i2c->twsi_base + SW_TWSI(i2c));

- cmd = SW_TWSI_V | SW_TWSI_R | SW_TWSI_SOVR;
- /* SIZE */
- cmd |= (u64)(msgs[1].len - 1) << SW_TWSI_SIZE_SHIFT;
- /* A */
- cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT;
+ return octeon_i2c_hlc_wait(i2c);
+}

- if (msgs[0].flags & I2C_M_TEN)
+/* Construct and send i2c transaction core cmd */
+static int octeon_i2c_hlc_cmd(struct octeon_i2c *i2c, struct i2c_msg msg, u64 cmd)
+{
+ if (msg.flags & I2C_M_TEN)
cmd |= SW_TWSI_OP_10_IA;
else
cmd |= SW_TWSI_OP_7_IA;

- if (msgs[0].len == 2) {
+ if (msg.len == 2) {
u64 ext = 0;

cmd |= SW_TWSI_EIA;
- ext = (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
- cmd |= (u64)msgs[0].buf[1] << SW_TWSI_IA_SHIFT;
+ ext = (u64)msg.buf[0] << SW_TWSI_IA_SHIFT;
+ cmd |= (u64)msg.buf[1] << SW_TWSI_IA_SHIFT;
octeon_i2c_writeq_flush(ext, i2c->twsi_base + SW_TWSI_EXT(i2c));
} else {
- cmd |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
+ cmd |= (u64)msg.buf[0] << SW_TWSI_IA_SHIFT;
}

- octeon_i2c_hlc_int_clear(i2c);
- octeon_i2c_writeq_flush(cmd, i2c->twsi_base + SW_TWSI(i2c));
+ return octeon_i2c_hlc_cmd_send(i2c, cmd);
+}

- ret = octeon_i2c_hlc_wait(i2c);
+/* high-level-controller composite write+read, msg0=addr, msg1=data */
+static int octeon_i2c_hlc_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs)
+{
+ int i, j, ret = 0;
+ u64 cmd;
+
+ octeon_i2c_hlc_enable(i2c);
+
+ cmd = SW_TWSI_V | SW_TWSI_R | SW_TWSI_SOVR;
+ /* SIZE */
+ cmd |= (u64)(msgs[1].len - 1) << SW_TWSI_SIZE_SHIFT;
+ /* A */
+ cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT;
+
+ /* Send core command */
+ ret = octeon_i2c_hlc_cmd(i2c, msgs[0], cmd);
if (ret)
goto err;

@@ -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;

@@ -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
+ * Used in the case where the i2c xfer is for greater than 8 bytes of read data.
+ */
+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 + TWSI_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;
+
+ /* Send core command */
+ ret = octeon_i2c_hlc_cmd(i2c, msgs[0], cmd);
+ if (ret)
+ return ret;
+
+ cmd = __raw_readq(i2c->twsi_base + SW_TWSI(i2c));
+ if ((cmd & SW_TWSI_R) == 0)
+ return octeon_i2c_check_status(i2c, false);
+
+ /* 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;
+ }
+
+ octeon_i2c_block_disable(i2c);
+ return ret;
+}
+
+/**
+ * high-level-controller composite block write+write, m[0]len<=2, m[1]len<=1024
+ * Used in the case where the i2c xfer is for greater than 8 bytes of write data.
+ */
+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 + TWSI_BLOCK_CTL(i2c));
+
+ /* Prepare core command */
+ cmd = SW_TWSI_V | SW_TWSI_SOVR;
+ cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT;
+
+ if (msgs[0].flags & I2C_M_TEN)
+ cmd |= SW_TWSI_OP_10_IA;
+ else
+ cmd |= SW_TWSI_OP_7_IA;
+
+ if (msgs[0].len == 2) {
+ cmd |= SW_TWSI_EIA;
+ ext |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
+ set_ext = true;
+ cmd |= (u64)msgs[0].buf[1] << SW_TWSI_IA_SHIFT;
+ } else {
+ cmd |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT;
+ }
+
+ /* 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));
+ 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;
+
+ 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;
+}
+
/**
* octeon_i2c_xfer - The driver's master_xfer function
* @adap: Pointer to the i2c_adapter structure
@@ -619,13 +749,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 && TWSI_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 9bb9f64fdda0..81fcf413c890 100644
--- a/drivers/i2c/busses/i2c-octeon-core.h
+++ b/drivers/i2c/busses/i2c-octeon-core.h
@@ -85,6 +85,11 @@
#define TWSI_INT_SDA BIT_ULL(10)
#define TWSI_INT_SCL BIT_ULL(11)

+#define TWSI_MODE_STRETCH BIT_ULL(1)
+#define TWSI_MODE_BLOCK_MODE BIT_ULL(2)
+
+#define TWSI_BLOCK_STS_RESET_PTR BIT_ULL(0)
+#define TWSI_BLOCK_STS_BUSY BIT_ULL(1)
#define I2C_OCTEON_EVENT_WAIT 80 /* microseconds */

/* Register offsets */
@@ -92,11 +97,19 @@ struct octeon_i2c_reg_offset {
unsigned int sw_twsi;
unsigned int twsi_int;
unsigned int sw_twsi_ext;
+ unsigned int twsi_mode;
+ unsigned int twsi_block_ctl;
+ unsigned int twsi_block_sts;
+ unsigned int twsi_block_fifo;
};

#define SW_TWSI(x) (x->roff.sw_twsi)
#define TWSI_INT(x) (x->roff.twsi_int)
#define SW_TWSI_EXT(x) (x->roff.sw_twsi_ext)
+#define TWSI_MODE(x) (x->roff.twsi_mode)
+#define TWSI_BLOCK_CTL(x) (x->roff.twsi_block_ctl)
+#define TWSI_BLOCK_STS(x) (x->roff.twsi_block_sts)
+#define TWSI_BLOCK_FIFO(x) (x->roff.twsi_block_fifo)

struct octeon_i2c {
wait_queue_head_t queue;
@@ -110,6 +123,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 a77cd86fe75e..abde98117d7e 100644
--- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c
+++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
@@ -165,6 +165,10 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
i2c->roff.sw_twsi = 0x1000;
i2c->roff.twsi_int = 0x1010;
i2c->roff.sw_twsi_ext = 0x1018;
+ i2c->roff.twsi_mode = 0x1038;
+ i2c->roff.twsi_block_ctl = 0x1048;
+ i2c->roff.twsi_block_sts = 0x1050;
+ i2c->roff.twsi_block_fifo = 0x1058;

i2c->dev = dev;
pci_set_drvdata(pdev, i2c);
--
2.43.2


2024-04-15 22:00:28

by Andi Shyti

[permalink] [raw]
Subject: Re: [PATCH v4] i2c: octeon: Add block-mode r/w

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 <[email protected]>

..

> +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;
> +}

..

2024-06-10 02:59:20

by Aryan Srivastava

[permalink] [raw]
Subject: Re: [PATCH v4] i2c: octeon: Add block-mode r/w

Hi Andi,

Thanks for your comments.
On Mon, 2024-04-15 at 23:59 +0200, Andi Shyti wrote:
> 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 :-)
>
Done.
> > Signed-off-by: Aryan Srivastava
> > <[email protected]>
>
> ...
>
> > +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).
This is simply to check if the HW is capable for block transactions.
>
> > +               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.
>
Done.
> >  
> > @@ -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.
>
Done.
> > + * 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?
Done, also added explanation in commit.
>
> > +       }
> > +
> > +       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?
>
Done.
> > +               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?
There is a disable at the bottom of the function for block mode.
>
> 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;
> > +}
>
> ...
Thanks,
Aryan.