| Message ID | 20210216182735.11639-1-jae.hyun.yoo@linux.intel.com |
|---|---|
| Headers | show |
| Series | i2c: aspeed: Add buffer and DMA modes support | expand |
On Tue, Feb 16, 2021 at 10:15 AM Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote: > > Append bindings to support transfer mode. > > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
On Tue, Feb 16, 2021 at 10:15 AM Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> 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@linux.intel.com> > Tested-by: Tao Ren <taoren@fb.com> 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; > +} > + [...]
Hi Brendan, On 2/23/2021 3:03 PM, Brendan Higgins wrote: > On Tue, Feb 16, 2021 at 10:15 AM Jae Hyun Yoo > <jae.hyun.yoo@linux.intel.com> 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@linux.intel.com> >> Tested-by: Tao Ren <taoren@fb.com> > > 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. 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 >> + 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; >> +} >> + > [...] >
This patch series adds buffer mode and DMA mode transfer support for the Aspeed I2C driver. With this change, buffer mode and DMA mode can be selectively used depend on platform configuration. * Buffer mode AST2400: It has 2 KBytes (256 Bytes x 8 pages) of I2C SRAM buffer pool from 0x1e78a800 to 0x1e78afff that can be used for all busses with buffer pool manipulation. To simplify implementation for supporting both AST2400 and AST2500, it assigns each 128 Bytes per bus without using buffer pool manipulation so total 1792 Bytes of I2C SRAM buffer will be used. AST2500: It has 16 Bytes of individual I2C SRAM buffer per each bus and its range is from 0x1e78a200 to 0x1e78a2df, so it doesn't have 'buffer page selection' bit field in the Function control register, and neither 'base address pointer' bit field in the Pool buffer control register it has. To simplify implementation for supporting both AST2400 and AST2500, it writes zeros on those register bit fields but it's okay because it does nothing in AST2500. AST2600: It has 32 Bytes of individual I2C SRAM buffer per each bus and its range is from 0x1e78ac00 to 0x1e78adff. Works just like AST2500 does. * DMA mode Only AST2500 and later versions support DMA mode under some limitations in case of AST2500: I2C is sharing the DMA H/W with UHCI host controller and MCTP controller. Since those controllers operate with DMA mode only, I2C has to use buffer mode or byte mode instead if one of those controllers is enabled. Also make sure that if SD/eMMC or Port80 snoop uses DMA mode instead of PIO or FIFO respectively, I2C can't use DMA mode. Please review it. Changes since v2: - Added SRAM resources back to default dtsi and added a mode selection property. - Refined SoC family dependent xfer mode configuration functions. Changes since v1: V1: https://lore.kernel.org/linux-arm-kernel/20191007231313.4700-1-jae.hyun.yoo@linux.intel.com/ - Removed a bug fix patch which was merged already from this patch series. - Removed buffer reg settings from default device tree and added the settings into bindings document to show the predefined buffer range per each bus. - Updated commit message and comments. - Refined driver code using abstract functions. Jae Hyun Yoo (4): dt-bindings: i2c: aspeed: add transfer mode support ARM: dts: aspeed: modify I2C node to support buffer mode i2c: aspeed: add buffer mode transfer support i2c: aspeed: add DMA mode transfer support .../devicetree/bindings/i2c/i2c-aspeed.txt | 37 +- arch/arm/boot/dts/aspeed-g4.dtsi | 47 +- arch/arm/boot/dts/aspeed-g5.dtsi | 47 +- arch/arm/boot/dts/aspeed-g6.dtsi | 32 +- drivers/i2c/busses/i2c-aspeed.c | 637 ++++++++++++++++-- 5 files changed, 684 insertions(+), 116 deletions(-)