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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[....]



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux