Re: [PATCH v4 4/4] i2c: aspeed: add DMA mode transfer support

[Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>]

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

[....]