Hi Brendan,
On 4/14/2021 8:08 AM, Jae Hyun Yoo wrote:
[....]
@@ -581,42 +660,55 @@ aspeed_i2c_master_handle_tx_first(struct
aspeed_i2c_bus *bus,
{
u32 command = 0;
- if (bus->buf_base) {
- u8 wbuf[4];
+ if (bus->dma_buf || bus->buf_base) {
int len;
- int i;
if (msg->len - bus->buf_index > bus->buf_size)
len = bus->buf_size;
else
len = msg->len - bus->buf_index;
- command |= ASPEED_I2CD_TX_BUFF_ENABLE;
+ if (bus->dma_buf) {
+ command |= ASPEED_I2CD_TX_DMA_ENABLE;
- if (msg->len - bus->buf_index > bus->buf_size)
- len = bus->buf_size;
- else
- len = msg->len - bus->buf_index;
+ memcpy(bus->dma_buf, msg->buf +
bus->buf_index, len);
- /*
- * Looks bad here again but use dword writings to
avoid data
- * corruption of byte writing on remapped I2C SRAM.
- */
- for (i = 0; i < len; i++) {
- wbuf[i % 4] = msg->buf[bus->buf_index + i];
- if (i % 4 == 3)
+ writel(bus->dma_handle &
ASPEED_I2CD_DMA_ADDR_MASK,
+ bus->base + ASPEED_I2C_DMA_ADDR_REG);
+ writel(FIELD_PREP(ASPEED_I2CD_DMA_LEN_MASK,
len),
+ bus->base + ASPEED_I2C_DMA_LEN_REG);
+ bus->dma_len = len;
+ } else {
+ u8 wbuf[4];
+ int i;
+
+ command |= ASPEED_I2CD_TX_BUFF_ENABLE;
+
+ if (msg->len - bus->buf_index > bus->buf_size)
+ len = bus->buf_size;
+ else
+ len = msg->len - bus->buf_index;
+
+ /*
+ * Looks bad here again but use dword
writings to avoid
+ * data corruption of byte writing on
remapped I2C SRAM.
+ */
+ for (i = 0; i < len; i++) {
+ wbuf[i % 4] = msg->buf[bus->buf_index
+ i];
+ if (i % 4 == 3)
+ writel(*(u32 *)wbuf,
+ bus->buf_base + i - 3);
+ }
+ 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));
+ 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);
+ 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;
} else {
Some of these functions are getting really complex and most of the
logic for the different modes is in different if-else blocks. Could
you look into splitting this into separate functions based on which
mode is being used?
Otherwise, this patch looks good.
I already splitted some big chunk of mode dependant logics to address
your comment on v1. Could you please check again the patched result of
this function? It's not much complex and it'd be better keep as is for
consistency in other changes in this patch. I think, splitting it again
would be not good for readability of the code flow.
Thanks,
Jae
This is the patched result of this function:
static inline u32
aspeed_i2c_master_handle_tx_first(struct aspeed_i2c_bus *bus,
struct i2c_msg *msg)
{
u32 command = 0;
if (bus->dma_buf || bus->buf_base) {
int len;
if (msg->len - bus->buf_index > bus->buf_size)
len = bus->buf_size;
else
len = msg->len - bus->buf_index;
if (bus->dma_buf) {
command |= ASPEED_I2CD_TX_DMA_ENABLE;
memcpy(bus->dma_buf, msg->buf + bus->buf_index, len);
writel(bus->dma_handle & ASPEED_I2CD_DMA_ADDR_MASK,
bus->base + ASPEED_I2C_DMA_ADDR_REG);
writel(FIELD_PREP(ASPEED_I2CD_DMA_LEN_MASK, len),
bus->base + ASPEED_I2C_DMA_LEN_REG);
bus->dma_len = len;
} else {
u8 wbuf[4];
int i;
command |= ASPEED_I2CD_TX_BUFF_ENABLE;
if (msg->len - bus->buf_index > bus->buf_size)
len = bus->buf_size;
else
len = msg->len - bus->buf_index;
for (i = 0; i < len; i++) {
wbuf[i % 4] = msg->buf[bus->buf_index + i];
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;
} else {
writel(msg->buf[bus->buf_index++],
bus->base + ASPEED_I2C_BYTE_BUF_REG);
}
return command;
}
Do you still think that it should be split into separate functions per
each transfer mode?
Thanks,
Jae
[....]
