Hi Brendan,

On 2/23/2021 3:03 PM, Brendan Higgins wrote:

On Tue, Feb 16, 2021 at 10:15 AM Jae Hyun Yoo
[off-list ref] wrote:

quoted

This driver uses byte mode that makes lots of interrupt calls
which isn't good for performance and it makes the driver very
timing sensitive. To improve performance of the driver, this commit
adds buffer mode transfer support which uses I2C SRAM buffer
instead of using a single byte buffer.

Signed-off-by: Jae Hyun Yoo <redacted>
Tested-by: Tao Ren <redacted>

Overall looks pretty good! There were just a couple bits of code which
were not immediately obvious to me that I would like to see improved:

quoted

---
Changes since v2:
- Refined SoC family dependent xfer mode configuration functions.

Changes since v1:
- Updated commit message.
- Refined using abstract functions.

  drivers/i2c/busses/i2c-aspeed.c | 464 ++++++++++++++++++++++++++++----
  1 file changed, 412 insertions(+), 52 deletions(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 724bf30600d6..343e621ff133 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c

[...]

quoted

+static inline u32
+aspeed_i2c_prepare_tx_buf(struct aspeed_i2c_bus *bus, struct i2c_msg *msg)
+{
+       u8 slave_addr = i2c_8bit_addr_from_msg(msg);
+       u32 command = 0;
+       int len;
+
+       if (msg->len + 1 > bus->buf_size)
+               len = bus->buf_size;
+       else
+               len = msg->len + 1;
+
+       if (bus->buf_base) {
+               u8 wbuf[4];
+               int i;
+
+               command |= ASPEED_I2CD_TX_BUFF_ENABLE;
+
+               /*
+                * Yeah, it looks bad but byte writing on remapped I2C SRAM
+                * causes corruption so use this way to make dword writings.
+                */

Not surprised. It looks like you reuse this code in a couple of
places, at the very least I think you should break this out into a
helper function. Otherwise, please make a similar comment in the other
locations.

There is one more place which has a similar code but loop count, tx
buffer and message buffer indexing are slightly different so better
leave them, IMO. Instead, I'll add this comment even for the other one.

Also, why doesn't writesl()
(https://elixir.bootlin.com/linux/v5.11/source/include/asm-generic/io.h#L413)
work here?

This is caused by Aspeed I2C SRAM specific behavior so it can't be
covered by writesl().

Will submit v4 soon. Thanks for your review!

Best,
Jae

quoted

+               wbuf[0] = slave_addr;
+               for (i = 1; i < len; i++) {
+                       wbuf[i % 4] = msg->buf[i - 1];
+                       if (i % 4 == 3)
+                               writel(*(u32 *)wbuf, bus->buf_base + i - 3);
+               }
+               if (--i % 4 != 3)
+                       writel(*(u32 *)wbuf, bus->buf_base + i - (i % 4));
+
+               writel(FIELD_PREP(ASPEED_I2CD_BUF_TX_COUNT_MASK, len - 1) |
+                      FIELD_PREP(ASPEED_I2CD_BUF_OFFSET_MASK, bus->buf_offset),
+                      bus->base + ASPEED_I2C_BUF_CTRL_REG);
+       }
+
+       bus->buf_index = len - 1;
+
+       return command;
+}
+

[...]