From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Date: Wed, 19 May 2021 11:38:56 -0700
Subject: [PATCH v4 4/4] i2c: aspeed: add DMA mode transfer support
In-Reply-To: <685d21f9-2f9d-c0ae-ee77-10eb4441870c@linux.intel.com>
References: <20210224191720.7724-1-jae.hyun.yoo@linux.intel.com>
 <20210224191720.7724-5-jae.hyun.yoo@linux.intel.com>
 <CAFd5g46eAcnSVFuUrXTuJmP-s_hsXDJ9x4QSQUM5JqfxFNwJVw@mail.gmail.com>
 <685d21f9-2f9d-c0ae-ee77-10eb4441870c@linux.intel.com>
Message-ID: <6ab00810-d99e-fcd8-091c-0961ae82f0ed@linux.intel.com>
List-Id: <linux-aspeed.lists.ozlabs.org>
To: linux-aspeed@lists.ozlabs.org
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

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

[....]