On Tue, Feb 16, 2021 at 10:15 AM Jae Hyun Yoo
<jae.hyun.yoo@xxxxxxxxxxxxxxx> wrote:
>
> 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 <jae.hyun.yoo@xxxxxxxxxxxxxxx>
> Tested-by: Tao Ren <taoren@xxxxxx>
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:
> ---
> 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
[...]
> +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.
Also, why doesn't writesl()
(https://elixir.bootlin.com/linux/v5.11/source/include/asm-generic/io.h#L413)
work here?
> + 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;
> +}
> +
[...]
