Hi Brendan,

On 4/14/2021 8:08 AM, Jae Hyun Yoo wrote:

[....]

quoted

quoted

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

[....]