[PATCH v2 0/4] i2c: aspeed: Add buffer and DMA modes support

[PATCH v2 0/4] i2c: aspeed: Add buffer and DMA modes support

[Jae Hyun Yoo]

This patch series adds buffer mode and DMA mode transfer support for the
Aspeed I2C driver. With this change, default transfer mode will be set to
buffer mode for better performance, and DMA mode can be selectively used
depends on platform configuration.

* Buffer mode
  AST2400:
    It has 2 KBytes (256 Bytes x 8 pages) of I2C SRAM buffer pool from
    0x1e78a800 to 0x1e78afff that can be used for all busses with
    buffer pool manipulation. To simplify implementation for supporting
    both AST2400 and AST2500, it assigns each 128 Bytes per bus without
    using buffer pool manipulation so total 1792 Bytes of I2C SRAM
    buffer will be used.

  AST2500:
    It has 16 Bytes of individual I2C SRAM buffer per each bus and its
    range is from 0x1e78a200 to 0x1e78a2df, so it doesn't have 'buffer
    page selection' bit field in the Function control register, and
    neither 'base address pointer' bit field in the Pool buffer control
    register it has. To simplify implementation for supporting both
    AST2400 and AST2500, it writes zeros on those register bit fields
    but it's okay because it does nothing in AST2500.

  AST2600:
    It has 32 Bytes of individual I2C SRAM buffer per each bus and its
    range is from 0x1e78ac00 to 0x1e78adff. Works just like AST2500
    does.

* DMA mode
  Only AST2500 and later versions support DMA mode under some limitations
  in case of AST2500:
    I2C is sharing the DMA H/W with UHCI host controller and MCTP
    controller. Since those controllers operate with DMA mode only, I2C
    has to use buffer mode or byte mode instead if one of those
    controllers is enabled. Also make sure that if SD/eMMC or Port80
    snoop uses DMA mode instead of PIO or FIFO respectively, I2C can't
    use DMA mode.

Please review it.

Changes since v1:
V1: https://lore.kernel.org/linux-arm-kernel/20191007231313.4700-1-jae.hyun.yoo@linux.intel.com/
- Removed a bug fix patch which was merged already from this patch series. 
- Removed buffer reg settings from default device tree and added the settings
  into bindings document to show the predefined buffer range per each bus.
- Updated commit message and comments.
- Refined driver code using abstract functions.

Jae Hyun Yoo (4):
  dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support
  ARM: dts: aspeed: modify I2C node to support buffer mode
  i2c: aspeed: add buffer mode transfer support
  i2c: aspeed: add DMA mode transfer support

 .../devicetree/bindings/i2c/i2c-aspeed.txt    | 126 +++-
 arch/arm/boot/dts/aspeed-g4.dtsi              |  19 +-
 arch/arm/boot/dts/aspeed-g5.dtsi              |  19 +-
 drivers/i2c/busses/i2c-aspeed.c               | 553 ++++++++++++++++--
 4 files changed, 667 insertions(+), 50 deletions(-)

-- 
2.17.1

[PATCH v2 1/4] dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support

[Jae Hyun Yoo]

Append bindings to support buffer mode and DMA mode transfer.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
Changes since v1:
- Removed buffer reg settings from default device tree and added the settings
  into here to show the predefined buffer range per each bus.

 .../devicetree/bindings/i2c/i2c-aspeed.txt    | 126 +++++++++++++++++-
 1 file changed, 119 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
index b47f6ccb196a..978e8402fdfc 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
@@ -3,7 +3,62 @@ Device tree configuration for the I2C busses on the AST24XX, AST25XX, and AST26X
 Required Properties:
 - #address-cells	: should be 1
 - #size-cells		: should be 0
-- reg			: address offset and range of bus
+- reg			: Address offset and range of bus registers.
+
+			  An additional SRAM buffer address offset and range is
+			  optional in case of enabling I2C dedicated SRAM for
+			  buffer mode transfer support. If the optional range
+			  is defined, buffer mode will be enabled.
+			  - AST2400
+			    &i2c0 { reg = <0x40 0x40>, <0x800 0x80>; };
+			    &i2c1 { reg = <0x80 0x40>, <0x880 0x80>; };
+			    &i2c2 { reg = <0xc0 0x40>, <0x900 0x80>; };
+			    &i2c3 { reg = <0x100 0x40>, <0x980 0x80>; };
+			    &i2c4 { reg = <0x140 0x40>, <0xa00 0x80>; };
+			    &i2c5 { reg = <0x180 0x40>, <0xa80 0x80>; };
+			    &i2c6 { reg = <0x1c0 0x40>, <0xb00 0x80>; };
+			    &i2c7 { reg = <0x300 0x40>, <0xb80 0x80>; };
+			    &i2c8 { reg = <0x340 0x40>, <0xc00 0x80>; };
+			    &i2c9 { reg = <0x380 0x40>, <0xc80 0x80>; };
+			    &i2c10 { reg = <0x3c0 0x40>, <0xd00 0x80>; };
+			    &i2c11 { reg = <0x400 0x40>, <0xd80 0x80>; };
+			    &i2c12 { reg = <0x440 0x40>, <0xe00 0x80>; };
+			    &i2c13 { reg = <0x480 0x40>, <0xe80 0x80>; };
+
+			  - AST2500
+			    &i2c0 { reg = <0x40 0x40>, <0x200 0x10>; };
+			    &i2c1 { reg = <0x80 0x40>, <0x210 0x10>; };
+			    &i2c2 { reg = <0xc0 0x40>, <0x220 0x10>; };
+			    &i2c3 { reg = <0x100 0x40>, <0x230 0x10>; };
+			    &i2c4 { reg = <0x140 0x40>, <0x240 0x10>; };
+			    &i2c5 { reg = <0x180 0x40>, <0x250 0x10>; };
+			    &i2c6 { reg = <0x1c0 0x40>, <0x260 0x10>; };
+			    &i2c7 { reg = <0x300 0x40>, <0x270 0x10>; };
+			    &i2c8 { reg = <0x340 0x40>, <0x280 0x10>; };
+			    &i2c9 { reg = <0x380 0x40>, <0x290 0x10>; };
+			    &i2c10 { reg = <0x3c0 0x40>, <0x2a0 0x10>; };
+			    &i2c11 { reg = <0x400 0x40>, <0x2b0 0x10>; };
+			    &i2c12 { reg = <0x440 0x40>, <0x2c0 0x10>; };
+			    &i2c13 { reg = <0x480 0x40>, <0x2d0 0x10>; };
+
+			  - AST2600
+			    &i2c0 { reg = <0x80 0x80>, <0xc00 0x20>; };
+			    &i2c1 { reg = <0x100 0x80>, <0xc20 0x20>; };
+			    &i2c2 { reg = <0x180 0x80>, <0xc40 0x20>; };
+			    &i2c3 { reg = <0x200 0x80>, <0xc60 0x20>; };
+			    &i2c4 { reg = <0x280 0x80>, <0xc80 0x20>; };
+			    &i2c5 { reg = <0x300 0x80>, <0xca0 0x20>; };
+			    &i2c6 { reg = <0x380 0x80>, <0xcc0 0x20>; };
+			    &i2c7 { reg = <0x400 0x80>, <0xce0 0x20>; };
+			    &i2c8 { reg = <0x480 0x80>, <0xd00 0x20>; };
+			    &i2c9 { reg = <0x500 0x80>, <0xd20 0x20>; };
+			    &i2c10 { reg = <0x580 0x80>, <0xd40 0x20>; };
+			    &i2c11 { reg = <0x600 0x80>, <0xd60 0x20>; };
+			    &i2c12 { reg = <0x680 0x80>, <0xd80 0x20>; };
+			    &i2c13 { reg = <0x700 0x80>, <0xda0 0x20>; };
+			    &i2c14 { reg = <0x780 0x80>, <0xdc0 0x20>; };
+			    &i2c15 { reg = <0x800 0x80>, <0xde0 0x20>; };
+
 - compatible		: should be "aspeed,ast2400-i2c-bus"
 			  or "aspeed,ast2500-i2c-bus"
 			  or "aspeed,ast2600-i2c-bus"
@@ -17,6 +72,25 @@ Optional Properties:
 - bus-frequency	: frequency of the bus clock in Hz defaults to 100 kHz when not
 		  specified
 - multi-master	: states that there is another master active on this bus.
+- aspeed,dma-buf-size	: size of DMA buffer.
+			    AST2400: N/A
+			    AST2500: 2 ~ 4095
+			    AST2600: 2 ~ 4096
+
+			  If both DMA and buffer modes are enabled in device
+			  tree, DMA mode will be selected.
+
+			  AST2500 has these restrictions:
+			    - If one of these controllers is enabled
+				* UHCI host controller
+				* MCTP controller
+			      I2C has to use buffer mode or byte mode instead
+			      since these controllers run only in DMA mode and
+			      I2C is sharing the same DMA H/W with them.
+			    - If one of these controllers uses DMA mode, I2C
+			      can't use DMA mode
+				* SD/eMMC
+				* Port80 snoop
 
 Example:
 
@@ -26,12 +100,21 @@ i2c {
 	#size-cells = <1>;
 	ranges = <0 0x1e78a000 0x1000>;
 
-	i2c_ic: interrupt-controller@0 {
-		#interrupt-cells = <1>;
-		compatible = "aspeed,ast2400-i2c-ic";
+	i2c_gr: i2c-global-regs@0 {
+		compatible = "aspeed,ast2500-i2c-gr", "syscon";
 		reg = <0x0 0x40>;
-		interrupts = <12>;
-		interrupt-controller;
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x0 0x40>;
+
+		i2c_ic: interrupt-controller@0 {
+			#interrupt-cells = <1>;
+			compatible = "aspeed,ast2500-i2c-ic";
+			reg = <0x0 0x4>;
+			interrupts = <12>;
+			interrupt-controller;
+		};
 	};
 
 	i2c0: i2c-bus@40 {
@@ -39,11 +122,40 @@ i2c {
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 		reg = <0x40 0x40>;
-		compatible = "aspeed,ast2400-i2c-bus";
+		compatible = "aspeed,ast2500-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
 		bus-frequency = <100000>;
 		interrupts = <0>;
 		interrupt-parent = <&i2c_ic>;
 	};
+
+	/* buffer mode transfer enabled */
+	i2c1: i2c-bus@80 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		#interrupt-cells = <1>;
+		reg = <0x80 0x40>, <0x210 0x10>;
+		compatible = "aspeed,ast2500-i2c-bus";
+		clocks = <&syscon ASPEED_CLK_APB>;
+		resets = <&syscon ASPEED_RESET_I2C>;
+		bus-frequency = <100000>;
+		interrupts = <1>;
+		interrupt-parent = <&i2c_ic>;
+	};
+
+	/* DMA mode transfer enabled */
+	i2c2: i2c-bus@c0 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		#interrupt-cells = <1>;
+		reg = <0xc0 0x40>;
+		aspeed,dma-buf-size = <4095>;
+		compatible = "aspeed,ast2500-i2c-bus";
+		clocks = <&syscon ASPEED_CLK_APB>;
+		resets = <&syscon ASPEED_RESET_I2C>;
+		bus-frequency = <100000>;
+		interrupts = <2>;
+		interrupt-parent = <&i2c_ic>;
+	};
 };
-- 
2.17.1

[PATCH v2 2/4] ARM: dts: aspeed: modify I2C node to support buffer mode

[Jae Hyun Yoo]

This driver uses byte mode that makes lots of interrupt calls
so it's not good for performance. Also, it makes the driver very
timing sensitive. To improve performance of the driver, this commit
modifies I2C node to support buffer mode which uses I2C SRAM buffer
instead of using a single byte buffer.

AST2400:
It has 2 KBytes (256 Bytes x 8 pages) of I2C SRAM buffer pool from
0x1e78a800 to 0x1e78afff that can be used for all busses with
buffer pool manipulation. To simplify implementation for supporting
both AST2400 and AST2500, it assigns each 128 Bytes per bus without
using buffer pool manipulation so total 1792 Bytes of I2C SRAM
buffer will be used.

AST2500:
It has 16 Bytes of individual I2C SRAM buffer per each bus and its
range is from 0x1e78a200 to 0x1e78a2df, so it doesn't have 'buffer
page selection' bit field in the Function control register, and
neither 'base address pointer' bit field in the Pool buffer control
register it has. To simplify implementation for supporting both
AST2400 and AST2500, it writes zeros on those register bit fields
but it's okay because it does nothing in AST2500.

AST2600:
It has 32 Bytes of individual I2C SRAM buffer per each bus and its
range is from 0x1e78ac00 to 0x1e78adff. Works just like AST2500
does.

See Documentation/devicetree/bindings/i2c/i2c-aspeed.txt for
enabling buffer mode details.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
---
Changes since v1:
- Updated commit message.
- Removed buffer reg settings to keep the default transfer mode as byte mode.

 arch/arm/boot/dts/aspeed-g4.dtsi | 19 ++++++++++++++-----
 arch/arm/boot/dts/aspeed-g5.dtsi | 19 ++++++++++++++-----
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index b3dafbc8caca..35876b633b08 100644
--- a/arch/arm/boot/dts/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed-g4.dtsi
@@ -442,12 +442,21 @@
 };
 
 &i2c {
-	i2c_ic: interrupt-controller@0 {
-		#interrupt-cells = <1>;
-		compatible = "aspeed,ast2400-i2c-ic";
+	i2c_gr: i2c-global-regs@0 {
+		compatible = "aspeed,ast2400-i2c-gr", "syscon";
 		reg = <0x0 0x40>;
-		interrupts = <12>;
-		interrupt-controller;
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x0 0x40>;
+
+		i2c_ic: interrupt-controller@0 {
+			#interrupt-cells = <1>;
+			compatible = "aspeed,ast2400-i2c-ic";
+			reg = <0x0 0x4>;
+			interrupts = <12>;
+			interrupt-controller;
+		};
 	};
 
 	i2c0: i2c-bus@40 {
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index 5bc0de0f3365..2ef4fbe43afe 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -565,12 +565,21 @@
 };
 
 &i2c {
-	i2c_ic: interrupt-controller@0 {
-		#interrupt-cells = <1>;
-		compatible = "aspeed,ast2500-i2c-ic";
+	i2c_gr: i2c-global-regs@0 {
+		compatible = "aspeed,ast2500-i2c-gr", "syscon";
 		reg = <0x0 0x40>;
-		interrupts = <12>;
-		interrupt-controller;
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x0 0x40>;
+
+		i2c_ic: interrupt-controller@0 {
+			#interrupt-cells = <1>;
+			compatible = "aspeed,ast2500-i2c-ic";
+			reg = <0x0 0x4>;
+			interrupts = <12>;
+			interrupt-controller;
+		};
 	};
 
 	i2c0: i2c-bus@40 {
-- 
2.17.1

[PATCH v2 3/4] i2c: aspeed: add buffer mode transfer support

[Jae Hyun Yoo]

This driver uses byte mode that makes lots of interrupt calls
which isn't good for performance and it makes the driver very
timing sensitive. To improve performance of the driver, this commit
adds buffer mode transfer support which uses I2C SRAM buffer
instead of using a single byte buffer.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Tested-by: Tao Ren <taoren@fb.com>
---
Changes since v1:
- Updated commit message.
- Refined using abstract functions.

 drivers/i2c/busses/i2c-aspeed.c | 375 +++++++++++++++++++++++++++++---
 1 file changed, 345 insertions(+), 30 deletions(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 724bf30600d6..223885e7a62f 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -7,6 +7,7 @@
  *  Copyright 2017 Google, Inc.
  */
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/err.h>
@@ -19,15 +20,24 @@
 #include <linux/irqchip/chained_irq.h>
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 #include <linux/reset.h>
 #include <linux/slab.h>
 
-/* I2C Register */
+/* I2C Global Registers */
+/* 0x00 : I2CG Interrupt Status Register  */
+/* 0x08 : I2CG Interrupt Target Assignment  */
+/* 0x0c : I2CG Global Control Register (AST2500)  */
+#define ASPEED_I2CG_GLOBAL_CTRL_REG			0x0c
+#define  ASPEED_I2CG_SRAM_BUFFER_EN			BIT(0)
+
+/* I2C Bus Registers */
 #define ASPEED_I2C_FUN_CTRL_REG				0x00
 #define ASPEED_I2C_AC_TIMING_REG1			0x04
 #define ASPEED_I2C_AC_TIMING_REG2			0x08
@@ -35,14 +45,12 @@
 #define ASPEED_I2C_INTR_STS_REG				0x10
 #define ASPEED_I2C_CMD_REG				0x14
 #define ASPEED_I2C_DEV_ADDR_REG				0x18
+#define ASPEED_I2C_BUF_CTRL_REG				0x1c
 #define ASPEED_I2C_BYTE_BUF_REG				0x20
 
-/* Global Register Definition */
-/* 0x00 : I2C Interrupt Status Register  */
-/* 0x08 : I2C Interrupt Target Assignment  */
-
 /* Device Register Definition */
 /* 0x00 : I2CD Function Control Register  */
+#define ASPEED_I2CD_BUFFER_PAGE_SEL_MASK		GENMASK(22, 20)
 #define ASPEED_I2CD_MULTI_MASTER_DIS			BIT(15)
 #define ASPEED_I2CD_SDA_DRIVE_1T_EN			BIT(8)
 #define ASPEED_I2CD_M_SDA_DRIVE_1T_EN			BIT(7)
@@ -103,6 +111,8 @@
 #define ASPEED_I2CD_BUS_RECOVER_CMD			BIT(11)
 
 /* Command Bit */
+#define ASPEED_I2CD_RX_BUFF_ENABLE			BIT(7)
+#define ASPEED_I2CD_TX_BUFF_ENABLE			BIT(6)
 #define ASPEED_I2CD_M_STOP_CMD				BIT(5)
 #define ASPEED_I2CD_M_S_RX_CMD_LAST			BIT(4)
 #define ASPEED_I2CD_M_RX_CMD				BIT(3)
@@ -119,6 +129,13 @@
 /* 0x18 : I2CD Slave Device Address Register   */
 #define ASPEED_I2CD_DEV_ADDR_MASK			GENMASK(6, 0)
 
+/* 0x1c : I2CD Buffer Control Register */
+/* Use 8-bits or 6-bits wide bit fileds to support both AST2400 and AST2500 */
+#define ASPEED_I2CD_BUF_RX_COUNT_MASK			GENMASK(31, 24)
+#define ASPEED_I2CD_BUF_RX_SIZE_MASK			GENMASK(23, 16)
+#define ASPEED_I2CD_BUF_TX_COUNT_MASK			GENMASK(15, 8)
+#define ASPEED_I2CD_BUF_OFFSET_MASK			GENMASK(5, 0)
+
 enum aspeed_i2c_master_state {
 	ASPEED_I2C_MASTER_INACTIVE,
 	ASPEED_I2C_MASTER_PENDING,
@@ -164,6 +181,11 @@ struct aspeed_i2c_bus {
 	int				master_xfer_result;
 	/* Multi-master */
 	bool				multi_master;
+	/* Buffer mode */
+	void __iomem			*buf_base;
+	u8				buf_offset;
+	u8				buf_page;
+	size_t				buf_size;
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	struct i2c_client		*slave;
 	enum aspeed_i2c_slave_state	slave_state;
@@ -241,6 +263,77 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
 }
 
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
+static inline void
+aspeed_i2c_slave_handle_rx_done(struct aspeed_i2c_bus *bus, u32 irq_status,
+				u8 *value)
+{
+	if (bus->buf_base &&
+	    bus->slave_state == ASPEED_I2C_SLAVE_WRITE_RECEIVED &&
+	    !(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))
+		*value = readb(bus->buf_base);
+	else
+		*value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
+}
+
+static inline void
+aspeed_i2c_slave_handle_normal_stop(struct aspeed_i2c_bus *bus, u32 irq_status,
+				    u8 *value)
+{
+	int i, len;
+
+	if (bus->slave_state == ASPEED_I2C_SLAVE_WRITE_RECEIVED &&
+	    irq_status & ASPEED_I2CD_INTR_RX_DONE) {
+		if (bus->buf_base) {
+			len = FIELD_GET(ASPEED_I2CD_BUF_RX_COUNT_MASK,
+					readl(bus->base +
+					      ASPEED_I2C_BUF_CTRL_REG));
+			for (i = 0; i < len; i++) {
+				*value = readb(bus->buf_base + i);
+				i2c_slave_event(bus->slave,
+						I2C_SLAVE_WRITE_RECEIVED,
+						value);
+			}
+		}
+	}
+}
+
+static inline void
+aspeed_i2c_slave_handle_write_requested(struct aspeed_i2c_bus *bus, u8 *value)
+{
+	if (bus->buf_base) {
+		writel(FIELD_PREP(ASPEED_I2CD_BUF_RX_SIZE_MASK,
+				  bus->buf_size - 1) |
+		       FIELD_PREP(ASPEED_I2CD_BUF_OFFSET_MASK,
+				  bus->buf_offset),
+		       bus->base + ASPEED_I2C_BUF_CTRL_REG);
+		writel(ASPEED_I2CD_RX_BUFF_ENABLE,
+		       bus->base + ASPEED_I2C_CMD_REG);
+	}
+}
+
+static inline void
+aspeed_i2c_slave_handle_write_received(struct aspeed_i2c_bus *bus, u8 *value)
+{
+	int i, len;
+
+	if (bus->buf_base) {
+		len = FIELD_GET(ASPEED_I2CD_BUF_RX_COUNT_MASK,
+				readl(bus->base +
+				      ASPEED_I2C_BUF_CTRL_REG));
+		for (i = 1; i < len; i++) {
+			*value = readb(bus->buf_base + i);
+			i2c_slave_event(bus->slave, I2C_SLAVE_WRITE_RECEIVED,
+					value);
+		}
+		writel(FIELD_PREP(ASPEED_I2CD_BUF_RX_SIZE_MASK,
+				  bus->buf_size - 1) |
+		       FIELD_PREP(ASPEED_I2CD_BUF_OFFSET_MASK, bus->buf_offset),
+		       bus->base + ASPEED_I2C_BUF_CTRL_REG);
+		writel(ASPEED_I2CD_RX_BUFF_ENABLE,
+		       bus->base + ASPEED_I2C_CMD_REG);
+	}
+}
+
 static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 {
 	u32 command, irq_handled = 0;
@@ -267,7 +360,7 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 
 	/* Slave was sent something. */
 	if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
-		value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
+		aspeed_i2c_slave_handle_rx_done(bus, irq_status, &value);
 		/* Handle address frame. */
 		if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
 			if (value & 0x1)
@@ -282,9 +375,11 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 
 	/* Slave was asked to stop. */
 	if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+		aspeed_i2c_slave_handle_normal_stop(bus, irq_status, &value);
 		irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
 		bus->slave_state = ASPEED_I2C_SLAVE_STOP;
 	}
+
 	if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
 	    bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
 		irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
@@ -314,9 +409,11 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 	case ASPEED_I2C_SLAVE_WRITE_REQUESTED:
 		bus->slave_state = ASPEED_I2C_SLAVE_WRITE_RECEIVED;
 		i2c_slave_event(slave, I2C_SLAVE_WRITE_REQUESTED, &value);
+		aspeed_i2c_slave_handle_write_requested(bus, &value);
 		break;
 	case ASPEED_I2C_SLAVE_WRITE_RECEIVED:
 		i2c_slave_event(slave, I2C_SLAVE_WRITE_RECEIVED, &value);
+		aspeed_i2c_slave_handle_write_received(bus, &value);
 		break;
 	case ASPEED_I2C_SLAVE_STOP:
 		i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
@@ -336,12 +433,76 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 }
 #endif /* CONFIG_I2C_SLAVE */
 
+static inline u32
+aspeed_i2c_prepare_rx_buf(struct aspeed_i2c_bus *bus, struct i2c_msg *msg)
+{
+	u32 command = 0;
+	int len;
+
+	if (msg->len > bus->buf_size) {
+		len = bus->buf_size;
+	} else {
+		len = msg->len;
+		command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
+	}
+
+	if (bus->buf_base) {
+		command |= ASPEED_I2CD_RX_BUFF_ENABLE;
+
+		writel(FIELD_PREP(ASPEED_I2CD_BUF_RX_SIZE_MASK, len - 1) |
+		       FIELD_PREP(ASPEED_I2CD_BUF_OFFSET_MASK, bus->buf_offset),
+		       bus->base + ASPEED_I2C_BUF_CTRL_REG);
+	}
+
+	return command;
+}
+
+static inline u32
+aspeed_i2c_prepare_tx_buf(struct aspeed_i2c_bus *bus, struct i2c_msg *msg)
+{
+	u8 slave_addr = i2c_8bit_addr_from_msg(msg);
+	u32 command = 0;
+	int len;
+
+	if (msg->len + 1 > bus->buf_size)
+		len = bus->buf_size;
+	else
+		len = msg->len + 1;
+
+	if (bus->buf_base) {
+		u8 wbuf[4];
+		int i;
+
+		command |= ASPEED_I2CD_TX_BUFF_ENABLE;
+
+		/*
+		 * Yeah, it looks bad but byte writing on remapped I2C SRAM
+		 * causes corruption so use this way to make dword writings.
+		 */
+		wbuf[0] = slave_addr;
+		for (i = 1; i < len; i++) {
+			wbuf[i % 4] = msg->buf[i - 1];
+			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 - 1;
+
+	return command;
+}
+
 /* precondition: bus.lock has been acquired. */
 static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
 {
 	u32 command = ASPEED_I2CD_M_START_CMD | ASPEED_I2CD_M_TX_CMD;
 	struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
-	u8 slave_addr = i2c_8bit_addr_from_msg(msg);
 
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	/*
@@ -360,12 +521,21 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
 
 	if (msg->flags & I2C_M_RD) {
 		command |= ASPEED_I2CD_M_RX_CMD;
-		/* Need to let the hardware know to NACK after RX. */
-		if (msg->len == 1 && !(msg->flags & I2C_M_RECV_LEN))
-			command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
+		if (!(msg->flags & I2C_M_RECV_LEN)) {
+			if (msg->len && bus->buf_base)
+				command |= aspeed_i2c_prepare_rx_buf(bus, msg);
+
+			/* Need to let the hardware know to NACK after RX. */
+			if (msg->len <= 1)
+				command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
+		}
+	} else if (msg->len && bus->buf_base) {
+		command |= aspeed_i2c_prepare_tx_buf(bus, msg);
 	}
 
-	writel(slave_addr, bus->base + ASPEED_I2C_BYTE_BUF_REG);
+	if (!(command & ASPEED_I2CD_TX_BUFF_ENABLE))
+		writel(i2c_8bit_addr_from_msg(msg),
+		       bus->base + ASPEED_I2C_BYTE_BUF_REG);
 	writel(command, bus->base + ASPEED_I2C_CMD_REG);
 }
 
@@ -400,6 +570,103 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
 	return 0;
 }
 
+static inline u32
+aspeed_i2c_master_handle_tx_first(struct aspeed_i2c_bus *bus,
+				  struct i2c_msg *msg)
+{
+	u32 command = 0;
+
+	if (bus->buf_base) {
+		u8 wbuf[4];
+		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 (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;
+}
+
+static inline void
+aspeed_i2c_master_handle_rx(struct aspeed_i2c_bus *bus, struct i2c_msg *msg)
+{
+	u8 recv_byte;
+	int len;
+
+	if (bus->buf_base) {
+		len = FIELD_GET(ASPEED_I2CD_BUF_RX_COUNT_MASK,
+				readl(bus->base + ASPEED_I2C_BUF_CTRL_REG));
+		memcpy_fromio(msg->buf + bus->buf_index, bus->buf_base, len);
+		bus->buf_index += len;
+	} else {
+		recv_byte = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
+		msg->buf[bus->buf_index++] = recv_byte;
+	}
+}
+
+static inline u32
+aspeed_i2c_master_handle_rx_next(struct aspeed_i2c_bus *bus,
+				 struct i2c_msg *msg)
+{
+	u32 command = 0;
+
+	if (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;
+			command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
+		}
+
+		command |= ASPEED_I2CD_RX_BUFF_ENABLE;
+
+		writel(FIELD_PREP(ASPEED_I2CD_BUF_RX_SIZE_MASK,
+				  len - 1) |
+		       FIELD_PREP(ASPEED_I2CD_BUF_TX_COUNT_MASK, 0) |
+		       FIELD_PREP(ASPEED_I2CD_BUF_OFFSET_MASK,
+				  bus->buf_offset),
+		       bus->base + ASPEED_I2C_BUF_CTRL_REG);
+	} else {
+		if (bus->buf_index + 1 == msg->len)
+			command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
+	}
+
+	return command;
+}
+
 static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 {
 	u32 irq_handled = 0, command = 0;
@@ -508,11 +775,10 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 		fallthrough;
 	case ASPEED_I2C_MASTER_TX_FIRST:
 		if (bus->buf_index < msg->len) {
+			command = ASPEED_I2CD_M_TX_CMD;
+			command |= aspeed_i2c_master_handle_tx_first(bus, msg);
+			writel(command, bus->base + ASPEED_I2C_CMD_REG);
 			bus->master_state = ASPEED_I2C_MASTER_TX;
-			writel(msg->buf[bus->buf_index++],
-			       bus->base + ASPEED_I2C_BYTE_BUF_REG);
-			writel(ASPEED_I2CD_M_TX_CMD,
-			       bus->base + ASPEED_I2C_CMD_REG);
 		} else {
 			aspeed_i2c_next_msg_or_stop(bus);
 		}
@@ -529,26 +795,26 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 		}
 		irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
 
-		recv_byte = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
-		msg->buf[bus->buf_index++] = recv_byte;
-
 		if (msg->flags & I2C_M_RECV_LEN) {
+			recv_byte = readl(bus->base +
+					ASPEED_I2C_BYTE_BUF_REG) >> 8;
 			if (unlikely(recv_byte > I2C_SMBUS_BLOCK_MAX)) {
 				bus->cmd_err = -EPROTO;
 				aspeed_i2c_do_stop(bus);
 				goto out_no_complete;
 			}
-			msg->len = recv_byte +
-					((msg->flags & I2C_CLIENT_PEC) ? 2 : 1);
+			msg->len = recv_byte + ((msg->flags & I2C_CLIENT_PEC) ?
+						2 : 1);
 			msg->flags &= ~I2C_M_RECV_LEN;
+		} else if (msg->len) {
+			aspeed_i2c_master_handle_rx(bus, msg);
 		}
 
 		if (bus->buf_index < msg->len) {
-			bus->master_state = ASPEED_I2C_MASTER_RX;
 			command = ASPEED_I2CD_M_RX_CMD;
-			if (bus->buf_index + 1 == msg->len)
-				command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
+			command |= aspeed_i2c_master_handle_rx_next(bus, msg);
 			writel(command, bus->base + ASPEED_I2C_CMD_REG);
+			bus->master_state = ASPEED_I2C_MASTER_RX;
 		} else {
 			aspeed_i2c_next_msg_or_stop(bus);
 		}
@@ -908,6 +1174,9 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus *bus,
 	if (ret < 0)
 		return ret;
 
+	fun_ctrl_reg |= FIELD_PREP(ASPEED_I2CD_BUFFER_PAGE_SEL_MASK,
+				   bus->buf_page);
+
 	if (of_property_read_bool(pdev->dev.of_node, "multi-master"))
 		bus->multi_master = true;
 	else
@@ -948,6 +1217,52 @@ static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)
 	return ret;
 }
 
+static void aspeed_i2c_set_xfer_mode(struct aspeed_i2c_bus *bus)
+{
+	struct platform_device *pdev = to_platform_device(bus->dev);
+	bool sram_enabled = true;
+	int ret;
+
+	/*
+	 * Enable I2C SRAM in case of AST2500.
+	 * SRAM is enabled by default in AST2400 and AST2600.
+	 */
+	if (of_device_is_compatible(pdev->dev.of_node,
+				    "aspeed,ast2500-i2c-bus")) {
+		struct regmap *gr_regmap = syscon_regmap_lookup_by_compatible("aspeed,ast2500-i2c-gr");
+
+		if (IS_ERR(gr_regmap))
+			ret = PTR_ERR(gr_regmap);
+		else
+			ret = regmap_update_bits(gr_regmap,
+						 ASPEED_I2CG_GLOBAL_CTRL_REG,
+						 ASPEED_I2CG_SRAM_BUFFER_EN,
+						 ASPEED_I2CG_SRAM_BUFFER_EN);
+
+		if (ret)
+			sram_enabled = false;
+	}
+
+	if (sram_enabled) {
+		struct resource *res = platform_get_resource(pdev,
+							     IORESOURCE_MEM, 1);
+
+		if (res && resource_size(res) >= 2)
+			bus->buf_base = devm_ioremap_resource(&pdev->dev, res);
+
+		if (!IS_ERR_OR_NULL(bus->buf_base)) {
+			bus->buf_size = resource_size(res);
+			if (of_device_is_compatible(pdev->dev.of_node,
+						    "aspeed,ast2400-i2c-bus")) {
+				bus->buf_page = ((res->start >> 8) &
+						 GENMASK(3, 0)) - 8;
+				bus->buf_offset = (res->start >> 2) &
+						  ASPEED_I2CD_BUF_OFFSET_MASK;
+			}
+		}
+	}
+}
+
 static const struct of_device_id aspeed_i2c_bus_of_table[] = {
 	{
 		.compatible = "aspeed,ast2400-i2c-bus",
@@ -970,18 +1285,18 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
 	const struct of_device_id *match;
 	struct aspeed_i2c_bus *bus;
 	struct clk *parent_clk;
-	struct resource *res;
 	int irq, ret;
 
 	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
 	if (!bus)
 		return -ENOMEM;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	bus->base = devm_ioremap_resource(&pdev->dev, res);
+	bus->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(bus->base))
 		return PTR_ERR(bus->base);
 
+	bus->dev = &pdev->dev;
+
 	parent_clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(parent_clk))
 		return PTR_ERR(parent_clk);
@@ -1012,6 +1327,8 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
 		bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
 				match->data;
 
+	aspeed_i2c_set_xfer_mode(bus);
+
 	/* Initialize the I2C adapter */
 	spin_lock_init(&bus->lock);
 	init_completion(&bus->cmd_complete);
@@ -1023,8 +1340,6 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
 	strlcpy(bus->adap.name, pdev->name, sizeof(bus->adap.name));
 	i2c_set_adapdata(&bus->adap, bus);
 
-	bus->dev = &pdev->dev;
-
 	/* Clean up any left over interrupt state. */
 	writel(0, bus->base + ASPEED_I2C_INTR_CTRL_REG);
 	writel(0xffffffff, bus->base + ASPEED_I2C_INTR_STS_REG);
@@ -1048,8 +1363,8 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, bus);
 
-	dev_info(bus->dev, "i2c bus %d registered, irq %d\n",
-		 bus->adap.nr, irq);
+	dev_info(bus->dev, "i2c bus %d registered (%s mode), irq %d\n",
+		 bus->adap.nr, bus->buf_base ? "buffer" : "byte", irq);
 
 	return 0;
 }
-- 
2.17.1

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

[Jae Hyun Yoo]

This commit adds DMA mode transfer support.

Only AST2500 and later versions supports DMA mode.

AST2500 has these restrictions:
  - If one of these controllers is enabled
      * UHCI host controller
      * MCTP controller
    I2C has to use buffer mode or byte mode instead
    since these controllers run only in DMA mode and
    I2C is sharing the same DMA H/W with them.
  - If one of these controllers uses DMA mode, I2C
    can't use DMA mode
      * SD/eMMC
      * Port80 snoop

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
Changes since v1:
- Updated commit message and comments.
- Refined using abstract functions.

 drivers/i2c/busses/i2c-aspeed.c | 258 ++++++++++++++++++++++++++------
 1 file changed, 215 insertions(+), 43 deletions(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 223885e7a62f..717d3ef68da2 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -10,6 +10,8 @@
 #include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/completion.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmapool.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>
@@ -47,6 +49,8 @@
 #define ASPEED_I2C_DEV_ADDR_REG				0x18
 #define ASPEED_I2C_BUF_CTRL_REG				0x1c
 #define ASPEED_I2C_BYTE_BUF_REG				0x20
+#define ASPEED_I2C_DMA_ADDR_REG				0x24
+#define ASPEED_I2C_DMA_LEN_REG				0x28
 
 /* Device Register Definition */
 /* 0x00 : I2CD Function Control Register  */
@@ -111,6 +115,8 @@
 #define ASPEED_I2CD_BUS_RECOVER_CMD			BIT(11)
 
 /* Command Bit */
+#define ASPEED_I2CD_RX_DMA_ENABLE			BIT(9)
+#define ASPEED_I2CD_TX_DMA_ENABLE			BIT(8)
 #define ASPEED_I2CD_RX_BUFF_ENABLE			BIT(7)
 #define ASPEED_I2CD_TX_BUFF_ENABLE			BIT(6)
 #define ASPEED_I2CD_M_STOP_CMD				BIT(5)
@@ -136,6 +142,14 @@
 #define ASPEED_I2CD_BUF_TX_COUNT_MASK			GENMASK(15, 8)
 #define ASPEED_I2CD_BUF_OFFSET_MASK			GENMASK(5, 0)
 
+/* 0x24 : I2CD DMA Mode Buffer Address Register */
+#define ASPEED_I2CD_DMA_ADDR_MASK			GENMASK(31, 2)
+#define ASPEED_I2CD_DMA_ALIGN				4
+
+/* 0x28 : I2CD DMA Transfer Length Register */
+#define ASPEED_I2CD_DMA_LEN_SHIFT			0
+#define ASPEED_I2CD_DMA_LEN_MASK			GENMASK(11, 0)
+
 enum aspeed_i2c_master_state {
 	ASPEED_I2C_MASTER_INACTIVE,
 	ASPEED_I2C_MASTER_PENDING,
@@ -185,6 +199,12 @@ struct aspeed_i2c_bus {
 	void __iomem			*buf_base;
 	u8				buf_offset;
 	u8				buf_page;
+	/* DMA mode */
+	struct dma_pool			*dma_pool;
+	dma_addr_t			dma_handle;
+	u8				*dma_buf;
+	size_t				dma_len;
+	/* Buffer/DMA mode */
 	size_t				buf_size;
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	struct i2c_client		*slave;
@@ -267,9 +287,13 @@ static inline void
 aspeed_i2c_slave_handle_rx_done(struct aspeed_i2c_bus *bus, u32 irq_status,
 				u8 *value)
 {
-	if (bus->buf_base &&
+	if (bus->dma_buf &&
 	    bus->slave_state == ASPEED_I2C_SLAVE_WRITE_RECEIVED &&
 	    !(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))
+		*value = bus->dma_buf[0];
+	else if (bus->buf_base &&
+		 bus->slave_state == ASPEED_I2C_SLAVE_WRITE_RECEIVED &&
+		 !(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))
 		*value = readb(bus->buf_base);
 	else
 		*value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
@@ -283,7 +307,18 @@ aspeed_i2c_slave_handle_normal_stop(struct aspeed_i2c_bus *bus, u32 irq_status,
 
 	if (bus->slave_state == ASPEED_I2C_SLAVE_WRITE_RECEIVED &&
 	    irq_status & ASPEED_I2CD_INTR_RX_DONE) {
-		if (bus->buf_base) {
+		if (bus->dma_buf) {
+			len = bus->buf_size -
+			      FIELD_GET(ASPEED_I2CD_DMA_LEN_MASK,
+					readl(bus->base +
+					      ASPEED_I2C_DMA_LEN_REG));
+			for (i = 0; i < len; i++) {
+				*value = bus->dma_buf[i];
+				i2c_slave_event(bus->slave,
+						I2C_SLAVE_WRITE_RECEIVED,
+						value);
+			}
+		} else if (bus->buf_base) {
 			len = FIELD_GET(ASPEED_I2CD_BUF_RX_COUNT_MASK,
 					readl(bus->base +
 					      ASPEED_I2C_BUF_CTRL_REG));
@@ -300,7 +335,14 @@ aspeed_i2c_slave_handle_normal_stop(struct aspeed_i2c_bus *bus, u32 irq_status,
 static inline void
 aspeed_i2c_slave_handle_write_requested(struct aspeed_i2c_bus *bus, u8 *value)
 {
-	if (bus->buf_base) {
+	if (bus->dma_buf) {
+		writel(bus->dma_handle & ASPEED_I2CD_DMA_ADDR_MASK,
+		       bus->base + ASPEED_I2C_DMA_ADDR_REG);
+		writel(FIELD_PREP(ASPEED_I2CD_DMA_LEN_MASK, bus->buf_size),
+		       bus->base + ASPEED_I2C_DMA_LEN_REG);
+		writel(ASPEED_I2CD_RX_DMA_ENABLE,
+		       bus->base + ASPEED_I2C_CMD_REG);
+	} else if (bus->buf_base) {
 		writel(FIELD_PREP(ASPEED_I2CD_BUF_RX_SIZE_MASK,
 				  bus->buf_size - 1) |
 		       FIELD_PREP(ASPEED_I2CD_BUF_OFFSET_MASK,
@@ -316,7 +358,23 @@ aspeed_i2c_slave_handle_write_received(struct aspeed_i2c_bus *bus, u8 *value)
 {
 	int i, len;
 
-	if (bus->buf_base) {
+	if (bus->dma_buf) {
+		len = bus->buf_size -
+		      FIELD_GET(ASPEED_I2CD_DMA_LEN_MASK,
+				readl(bus->base +
+				      ASPEED_I2C_DMA_LEN_REG));
+		for (i = 1; i < len; i++) {
+			*value = bus->dma_buf[i];
+			i2c_slave_event(bus->slave, I2C_SLAVE_WRITE_RECEIVED,
+					value);
+		}
+		writel(bus->dma_handle & ASPEED_I2CD_DMA_ADDR_MASK,
+		       bus->base + ASPEED_I2C_DMA_ADDR_REG);
+		writel(FIELD_PREP(ASPEED_I2CD_DMA_LEN_MASK, bus->buf_size),
+		       bus->base + ASPEED_I2C_DMA_LEN_REG);
+		writel(ASPEED_I2CD_RX_DMA_ENABLE,
+		       bus->base + ASPEED_I2C_CMD_REG);
+	} else if (bus->buf_base) {
 		len = FIELD_GET(ASPEED_I2CD_BUF_RX_COUNT_MASK,
 				readl(bus->base +
 				      ASPEED_I2C_BUF_CTRL_REG));
@@ -446,7 +504,15 @@ aspeed_i2c_prepare_rx_buf(struct aspeed_i2c_bus *bus, struct i2c_msg *msg)
 		command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
 	}
 
-	if (bus->buf_base) {
+	if (bus->dma_buf) {
+		command |= ASPEED_I2CD_RX_DMA_ENABLE;
+
+		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 {
 		command |= ASPEED_I2CD_RX_BUFF_ENABLE;
 
 		writel(FIELD_PREP(ASPEED_I2CD_BUF_RX_SIZE_MASK, len - 1) |
@@ -469,7 +535,18 @@ aspeed_i2c_prepare_tx_buf(struct aspeed_i2c_bus *bus, struct i2c_msg *msg)
 	else
 		len = msg->len + 1;
 
-	if (bus->buf_base) {
+	if (bus->dma_buf) {
+		command |= ASPEED_I2CD_TX_DMA_ENABLE;
+
+		bus->dma_buf[0] = slave_addr;
+		memcpy(bus->dma_buf + 1, msg->buf, 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;
 
@@ -522,18 +599,19 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
 	if (msg->flags & I2C_M_RD) {
 		command |= ASPEED_I2CD_M_RX_CMD;
 		if (!(msg->flags & I2C_M_RECV_LEN)) {
-			if (msg->len && bus->buf_base)
+			if (msg->len && (bus->dma_buf || bus->buf_base))
 				command |= aspeed_i2c_prepare_rx_buf(bus, msg);
 
 			/* Need to let the hardware know to NACK after RX. */
 			if (msg->len <= 1)
 				command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
 		}
-	} else if (msg->len && bus->buf_base) {
+	} else if (msg->len && (bus->dma_buf || bus->buf_base)) {
 		command |= aspeed_i2c_prepare_tx_buf(bus, msg);
 	}
 
-	if (!(command & ASPEED_I2CD_TX_BUFF_ENABLE))
+	if (!(command & (ASPEED_I2CD_TX_BUFF_ENABLE |
+			 ASPEED_I2CD_TX_DMA_ENABLE)))
 		writel(i2c_8bit_addr_from_msg(msg),
 		       bus->base + ASPEED_I2C_BYTE_BUF_REG);
 	writel(command, bus->base + ASPEED_I2C_CMD_REG);
@@ -576,38 +654,51 @@ 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);
 
-		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;
+
+			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 {
@@ -624,7 +715,14 @@ aspeed_i2c_master_handle_rx(struct aspeed_i2c_bus *bus, struct i2c_msg *msg)
 	u8 recv_byte;
 	int len;
 
-	if (bus->buf_base) {
+	if (bus->dma_buf) {
+		len = bus->dma_len -
+		      FIELD_GET(ASPEED_I2CD_DMA_LEN_MASK,
+				readl(bus->base + ASPEED_I2C_DMA_LEN_REG));
+
+		memcpy(msg->buf + bus->buf_index, bus->dma_buf, len);
+		bus->buf_index += len;
+	} else if (bus->buf_base) {
 		len = FIELD_GET(ASPEED_I2CD_BUF_RX_COUNT_MASK,
 				readl(bus->base + ASPEED_I2C_BUF_CTRL_REG));
 		memcpy_fromio(msg->buf + bus->buf_index, bus->buf_base, len);
@@ -641,7 +739,7 @@ aspeed_i2c_master_handle_rx_next(struct aspeed_i2c_bus *bus,
 {
 	u32 command = 0;
 
-	if (bus->buf_base) {
+	if (bus->dma_buf || bus->buf_base) {
 		int len;
 
 		if (msg->len - bus->buf_index > bus->buf_size) {
@@ -651,14 +749,24 @@ aspeed_i2c_master_handle_rx_next(struct aspeed_i2c_bus *bus,
 			command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
 		}
 
-		command |= ASPEED_I2CD_RX_BUFF_ENABLE;
+		if (bus->dma_buf) {
+			command |= ASPEED_I2CD_RX_DMA_ENABLE;
 
-		writel(FIELD_PREP(ASPEED_I2CD_BUF_RX_SIZE_MASK,
-				  len - 1) |
-		       FIELD_PREP(ASPEED_I2CD_BUF_TX_COUNT_MASK, 0) |
-		       FIELD_PREP(ASPEED_I2CD_BUF_OFFSET_MASK,
-				  bus->buf_offset),
-		       bus->base + ASPEED_I2C_BUF_CTRL_REG);
+			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 {
+			command |= ASPEED_I2CD_RX_BUFF_ENABLE;
+
+			writel(FIELD_PREP(ASPEED_I2CD_BUF_RX_SIZE_MASK,
+					  len - 1) |
+			       FIELD_PREP(ASPEED_I2CD_BUF_TX_COUNT_MASK, 0) |
+			       FIELD_PREP(ASPEED_I2CD_BUF_OFFSET_MASK,
+					  bus->buf_offset),
+			       bus->base + ASPEED_I2C_BUF_CTRL_REG);
+		}
 	} else {
 		if (bus->buf_index + 1 == msg->len)
 			command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
@@ -1243,7 +1351,58 @@ static void aspeed_i2c_set_xfer_mode(struct aspeed_i2c_bus *bus)
 			sram_enabled = false;
 	}
 
+	/*
+	 * AST2400 doesn't support DMA mode.
+	 * AST2500 supports DMA mode under some restrictions:
+	 * I2C is sharing the DMA H/W with UHCI host controller and MCTP
+	 * controller. Since those controllers operate with DMA mode only, I2C
+	 * has to use buffer mode or byte mode instead if one of those
+	 * controllers is enabled. Also make sure that if SD/eMMC or Port80
+	 * snoop uses DMA mode instead of PIO or FIFO respectively, I2C can't
+	 * use DMA mode.
+	 */
 	if (sram_enabled) {
+		if (of_device_is_compatible(pdev->dev.of_node,
+					    "aspeed,ast2400-i2c-bus") ||
+		    (of_device_is_compatible(pdev->dev.of_node,
+					      "aspeed,ast2500-i2c-bus") &&
+		     IS_ENABLED(CONFIG_USB_UHCI_ASPEED))) {
+			sram_enabled = false;
+		} else {
+			u32 dma_len_max = ASPEED_I2CD_DMA_LEN_MASK >>
+					  ASPEED_I2CD_DMA_LEN_SHIFT;
+
+			ret = device_property_read_u32(&pdev->dev,
+						       "aspeed,dma-buf-size",
+						       &bus->buf_size);
+			if (!ret && bus->buf_size > dma_len_max)
+				bus->buf_size = dma_len_max;
+		}
+	}
+
+	if (bus->buf_size) {
+		if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(32))) {
+			dev_warn(&pdev->dev, "No suitable DMA available\n");
+		} else {
+			bus->dma_pool = dma_pool_create("i2c-aspeed",
+							&pdev->dev,
+							bus->buf_size,
+							ASPEED_I2CD_DMA_ALIGN,
+							0);
+			if (bus->dma_pool)
+				bus->dma_buf = dma_pool_alloc(bus->dma_pool,
+							      GFP_KERNEL,
+							      &bus->dma_handle);
+
+			if (!bus->dma_buf) {
+				dev_warn(&pdev->dev,
+					 "Cannot allocate DMA buffer\n");
+				dma_pool_destroy(bus->dma_pool);
+			}
+		}
+	}
+
+	if (!bus->dma_buf && sram_enabled) {
 		struct resource *res = platform_get_resource(pdev,
 							     IORESOURCE_MEM, 1);
 
@@ -1349,24 +1508,33 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
 	 */
 	ret = aspeed_i2c_init(bus, pdev);
 	if (ret < 0)
-		return ret;
+		goto out_free_dma_buf;
 
 	irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
 	ret = devm_request_irq(&pdev->dev, irq, aspeed_i2c_bus_irq,
 			       0, dev_name(&pdev->dev), bus);
 	if (ret < 0)
-		return ret;
+		goto out_free_dma_buf;
 
 	ret = i2c_add_adapter(&bus->adap);
 	if (ret < 0)
-		return ret;
+		goto out_free_dma_buf;
 
 	platform_set_drvdata(pdev, bus);
 
 	dev_info(bus->dev, "i2c bus %d registered (%s mode), irq %d\n",
-		 bus->adap.nr, bus->buf_base ? "buffer" : "byte", irq);
+		 bus->adap.nr, bus->dma_buf ? "dma" :
+					      bus->buf_base ? "buffer" : "byte",
+		 irq);
 
 	return 0;
+
+out_free_dma_buf:
+	if (bus->dma_buf)
+		dma_pool_free(bus->dma_pool, bus->dma_buf, bus->dma_handle);
+	dma_pool_destroy(bus->dma_pool);
+
+	return ret;
 }
 
 static int aspeed_i2c_remove_bus(struct platform_device *pdev)
@@ -1384,6 +1552,10 @@ static int aspeed_i2c_remove_bus(struct platform_device *pdev)
 
 	reset_control_assert(bus->rst);
 
+	if (bus->dma_buf)
+		dma_pool_free(bus->dma_pool, bus->dma_buf, bus->dma_handle);
+	dma_pool_destroy(bus->dma_pool);
+
 	i2c_del_adapter(&bus->adap);
 
 	return 0;
-- 
2.17.1

Re: [PATCH v2 1/4] dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support

[Rob Herring]

On Mon, Jan 11, 2021 at 04:37:46PM -0800, Jae Hyun Yoo wrote:
> Append bindings to support buffer mode and DMA mode transfer.
> 
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
> Changes since v1:
> - Removed buffer reg settings from default device tree and added the settings
>   into here to show the predefined buffer range per each bus.
> 
>  .../devicetree/bindings/i2c/i2c-aspeed.txt    | 126 +++++++++++++++++-
>  1 file changed, 119 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> index b47f6ccb196a..978e8402fdfc 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> @@ -3,7 +3,62 @@ Device tree configuration for the I2C busses on the AST24XX, AST25XX, and AST26X
>  Required Properties:
>  - #address-cells	: should be 1
>  - #size-cells		: should be 0
> -- reg			: address offset and range of bus
> +- reg			: Address offset and range of bus registers.
> +
> +			  An additional SRAM buffer address offset and range is
> +			  optional in case of enabling I2C dedicated SRAM for
> +			  buffer mode transfer support. If the optional range
> +			  is defined, buffer mode will be enabled.
> +			  - AST2400
> +			    &i2c0 { reg = <0x40 0x40>, <0x800 0x80>; };
> +			    &i2c1 { reg = <0x80 0x40>, <0x880 0x80>; };
> +			    &i2c2 { reg = <0xc0 0x40>, <0x900 0x80>; };
> +			    &i2c3 { reg = <0x100 0x40>, <0x980 0x80>; };
> +			    &i2c4 { reg = <0x140 0x40>, <0xa00 0x80>; };
> +			    &i2c5 { reg = <0x180 0x40>, <0xa80 0x80>; };
> +			    &i2c6 { reg = <0x1c0 0x40>, <0xb00 0x80>; };
> +			    &i2c7 { reg = <0x300 0x40>, <0xb80 0x80>; };
> +			    &i2c8 { reg = <0x340 0x40>, <0xc00 0x80>; };
> +			    &i2c9 { reg = <0x380 0x40>, <0xc80 0x80>; };
> +			    &i2c10 { reg = <0x3c0 0x40>, <0xd00 0x80>; };
> +			    &i2c11 { reg = <0x400 0x40>, <0xd80 0x80>; };
> +			    &i2c12 { reg = <0x440 0x40>, <0xe00 0x80>; };
> +			    &i2c13 { reg = <0x480 0x40>, <0xe80 0x80>; };

All this information doesn't need to be in the binding.

It's also an oddly structured dts file if this is what you are doing...

> +
> +			  - AST2500
> +			    &i2c0 { reg = <0x40 0x40>, <0x200 0x10>; };
> +			    &i2c1 { reg = <0x80 0x40>, <0x210 0x10>; };
> +			    &i2c2 { reg = <0xc0 0x40>, <0x220 0x10>; };
> +			    &i2c3 { reg = <0x100 0x40>, <0x230 0x10>; };
> +			    &i2c4 { reg = <0x140 0x40>, <0x240 0x10>; };
> +			    &i2c5 { reg = <0x180 0x40>, <0x250 0x10>; };
> +			    &i2c6 { reg = <0x1c0 0x40>, <0x260 0x10>; };
> +			    &i2c7 { reg = <0x300 0x40>, <0x270 0x10>; };
> +			    &i2c8 { reg = <0x340 0x40>, <0x280 0x10>; };
> +			    &i2c9 { reg = <0x380 0x40>, <0x290 0x10>; };
> +			    &i2c10 { reg = <0x3c0 0x40>, <0x2a0 0x10>; };
> +			    &i2c11 { reg = <0x400 0x40>, <0x2b0 0x10>; };
> +			    &i2c12 { reg = <0x440 0x40>, <0x2c0 0x10>; };
> +			    &i2c13 { reg = <0x480 0x40>, <0x2d0 0x10>; };
> +
> +			  - AST2600
> +			    &i2c0 { reg = <0x80 0x80>, <0xc00 0x20>; };
> +			    &i2c1 { reg = <0x100 0x80>, <0xc20 0x20>; };
> +			    &i2c2 { reg = <0x180 0x80>, <0xc40 0x20>; };
> +			    &i2c3 { reg = <0x200 0x80>, <0xc60 0x20>; };
> +			    &i2c4 { reg = <0x280 0x80>, <0xc80 0x20>; };
> +			    &i2c5 { reg = <0x300 0x80>, <0xca0 0x20>; };
> +			    &i2c6 { reg = <0x380 0x80>, <0xcc0 0x20>; };
> +			    &i2c7 { reg = <0x400 0x80>, <0xce0 0x20>; };
> +			    &i2c8 { reg = <0x480 0x80>, <0xd00 0x20>; };
> +			    &i2c9 { reg = <0x500 0x80>, <0xd20 0x20>; };
> +			    &i2c10 { reg = <0x580 0x80>, <0xd40 0x20>; };
> +			    &i2c11 { reg = <0x600 0x80>, <0xd60 0x20>; };
> +			    &i2c12 { reg = <0x680 0x80>, <0xd80 0x20>; };
> +			    &i2c13 { reg = <0x700 0x80>, <0xda0 0x20>; };
> +			    &i2c14 { reg = <0x780 0x80>, <0xdc0 0x20>; };
> +			    &i2c15 { reg = <0x800 0x80>, <0xde0 0x20>; };
> +
>  - compatible		: should be "aspeed,ast2400-i2c-bus"
>  			  or "aspeed,ast2500-i2c-bus"
>  			  or "aspeed,ast2600-i2c-bus"
> @@ -17,6 +72,25 @@ Optional Properties:
>  - bus-frequency	: frequency of the bus clock in Hz defaults to 100 kHz when not
>  		  specified
>  - multi-master	: states that there is another master active on this bus.
> +- aspeed,dma-buf-size	: size of DMA buffer.
> +			    AST2400: N/A
> +			    AST2500: 2 ~ 4095
> +			    AST2600: 2 ~ 4096

If based on the SoC, then all this can be implied from the compatible 
string.

> +
> +			  If both DMA and buffer modes are enabled in device
> +			  tree, DMA mode will be selected.
> +
> +			  AST2500 has these restrictions:
> +			    - If one of these controllers is enabled
> +				* UHCI host controller
> +				* MCTP controller
> +			      I2C has to use buffer mode or byte mode instead
> +			      since these controllers run only in DMA mode and
> +			      I2C is sharing the same DMA H/W with them.
> +			    - If one of these controllers uses DMA mode, I2C
> +			      can't use DMA mode
> +				* SD/eMMC
> +				* Port80 snoop
>  
>  Example:
>  
> @@ -26,12 +100,21 @@ i2c {
>  	#size-cells = <1>;
>  	ranges = <0 0x1e78a000 0x1000>;
>  
> -	i2c_ic: interrupt-controller@0 {
> -		#interrupt-cells = <1>;
> -		compatible = "aspeed,ast2400-i2c-ic";
> +	i2c_gr: i2c-global-regs@0 {
> +		compatible = "aspeed,ast2500-i2c-gr", "syscon";
>  		reg = <0x0 0x40>;
> -		interrupts = <12>;
> -		interrupt-controller;
> +
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0x0 0x0 0x40>;
> +
> +		i2c_ic: interrupt-controller@0 {
> +			#interrupt-cells = <1>;
> +			compatible = "aspeed,ast2500-i2c-ic";
> +			reg = <0x0 0x4>;
> +			interrupts = <12>;
> +			interrupt-controller;
> +		};
>  	};
>  
>  	i2c0: i2c-bus@40 {
> @@ -39,11 +122,40 @@ i2c {
>  		#size-cells = <0>;
>  		#interrupt-cells = <1>;
>  		reg = <0x40 0x40>;
> -		compatible = "aspeed,ast2400-i2c-bus";
> +		compatible = "aspeed,ast2500-i2c-bus";
>  		clocks = <&syscon ASPEED_CLK_APB>;
>  		resets = <&syscon ASPEED_RESET_I2C>;
>  		bus-frequency = <100000>;
>  		interrupts = <0>;
>  		interrupt-parent = <&i2c_ic>;
>  	};
> +
> +	/* buffer mode transfer enabled */
> +	i2c1: i2c-bus@80 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		#interrupt-cells = <1>;
> +		reg = <0x80 0x40>, <0x210 0x10>;
> +		compatible = "aspeed,ast2500-i2c-bus";
> +		clocks = <&syscon ASPEED_CLK_APB>;
> +		resets = <&syscon ASPEED_RESET_I2C>;
> +		bus-frequency = <100000>;
> +		interrupts = <1>;
> +		interrupt-parent = <&i2c_ic>;
> +	};
> +
> +	/* DMA mode transfer enabled */
> +	i2c2: i2c-bus@c0 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		#interrupt-cells = <1>;
> +		reg = <0xc0 0x40>;
> +		aspeed,dma-buf-size = <4095>;
> +		compatible = "aspeed,ast2500-i2c-bus";
> +		clocks = <&syscon ASPEED_CLK_APB>;
> +		resets = <&syscon ASPEED_RESET_I2C>;
> +		bus-frequency = <100000>;
> +		interrupts = <2>;
> +		interrupt-parent = <&i2c_ic>;
> +	};
>  };
> -- 
> 2.17.1
> 

Re: [PATCH v2 1/4] dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support

[Jae Hyun Yoo]

Hi Rob,

On 1/14/2021 11:34 AM, Rob Herring wrote:
>> -- reg			: address offset and range of bus
>> +- reg			: Address offset and range of bus registers.
>> +
>> +			  An additional SRAM buffer address offset and range is
>> +			  optional in case of enabling I2C dedicated SRAM for
>> +			  buffer mode transfer support. If the optional range
>> +			  is defined, buffer mode will be enabled.
>> +			  - AST2400
>> +			    &i2c0 { reg = <0x40 0x40>, <0x800 0x80>; };
>> +			    &i2c1 { reg = <0x80 0x40>, <0x880 0x80>; };
>> +			    &i2c2 { reg = <0xc0 0x40>, <0x900 0x80>; };
>> +			    &i2c3 { reg = <0x100 0x40>, <0x980 0x80>; };
>> +			    &i2c4 { reg = <0x140 0x40>, <0xa00 0x80>; };
>> +			    &i2c5 { reg = <0x180 0x40>, <0xa80 0x80>; };
>> +			    &i2c6 { reg = <0x1c0 0x40>, <0xb00 0x80>; };
>> +			    &i2c7 { reg = <0x300 0x40>, <0xb80 0x80>; };
>> +			    &i2c8 { reg = <0x340 0x40>, <0xc00 0x80>; };
>> +			    &i2c9 { reg = <0x380 0x40>, <0xc80 0x80>; };
>> +			    &i2c10 { reg = <0x3c0 0x40>, <0xd00 0x80>; };
>> +			    &i2c11 { reg = <0x400 0x40>, <0xd80 0x80>; };
>> +			    &i2c12 { reg = <0x440 0x40>, <0xe00 0x80>; };
>> +			    &i2c13 { reg = <0x480 0x40>, <0xe80 0x80>; };
> 
> All this information doesn't need to be in the binding.
> 
> It's also an oddly structured dts file if this is what you are doing...

I removed the default buffer mode settings that I added into
'aspeed-g4.dtsi' and 'aspeed-g5.dtsi' in v1 to avoid touching of the
default transfer mode setting, but each bus should use its dedicated
SRAM buffer range for enabling buffer mode so I added this information
at here as overriding examples instead. I thought that binding document
is a right place for providing this information but looks like it's not.
Any recommended place for it? Is it good enough if I add it just into
the commit message?

>> @@ -17,6 +72,25 @@ Optional Properties:
>>   - bus-frequency	: frequency of the bus clock in Hz defaults to 100 kHz when not
>>   		  specified
>>   - multi-master	: states that there is another master active on this bus.
>> +- aspeed,dma-buf-size	: size of DMA buffer.
>> +			    AST2400: N/A
>> +			    AST2500: 2 ~ 4095
>> +			    AST2600: 2 ~ 4096
> 
> If based on the SoC, then all this can be implied from the compatible
> string.
> 

Please help me to clarify your comment. Should I remove it from here
with keeping the driver handling code for each SoC compatible string?
Or should I change it like below?
aspeed,ast2400-i2c-bus: N/A
aspeed,ast2500-i2c-bus: 2 ~ 4095
aspeed,ast2600-i2c-bus: 2 ~ 4096

Thanks,
Jae

Re: [PATCH v2 1/4] dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support

[Joel Stanley]

On Thu, 14 Jan 2021 at 20:05, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote:
>
> Hi Rob,
>
> On 1/14/2021 11:34 AM, Rob Herring wrote:
> >> -- reg                       : address offset and range of bus
> >> +- reg                       : Address offset and range of bus registers.
> >> +
> >> +                      An additional SRAM buffer address offset and range is
> >> +                      optional in case of enabling I2C dedicated SRAM for
> >> +                      buffer mode transfer support. If the optional range
> >> +                      is defined, buffer mode will be enabled.
> >> +                      - AST2400
> >> +                        &i2c0 { reg = <0x40 0x40>, <0x800 0x80>; };
> >> +                        &i2c1 { reg = <0x80 0x40>, <0x880 0x80>; };
> >> +                        &i2c2 { reg = <0xc0 0x40>, <0x900 0x80>; };
> >> +                        &i2c3 { reg = <0x100 0x40>, <0x980 0x80>; };
> >> +                        &i2c4 { reg = <0x140 0x40>, <0xa00 0x80>; };
> >> +                        &i2c5 { reg = <0x180 0x40>, <0xa80 0x80>; };
> >> +                        &i2c6 { reg = <0x1c0 0x40>, <0xb00 0x80>; };
> >> +                        &i2c7 { reg = <0x300 0x40>, <0xb80 0x80>; };
> >> +                        &i2c8 { reg = <0x340 0x40>, <0xc00 0x80>; };
> >> +                        &i2c9 { reg = <0x380 0x40>, <0xc80 0x80>; };
> >> +                        &i2c10 { reg = <0x3c0 0x40>, <0xd00 0x80>; };
> >> +                        &i2c11 { reg = <0x400 0x40>, <0xd80 0x80>; };
> >> +                        &i2c12 { reg = <0x440 0x40>, <0xe00 0x80>; };
> >> +                        &i2c13 { reg = <0x480 0x40>, <0xe80 0x80>; };
> >
> > All this information doesn't need to be in the binding.
> >
> > It's also an oddly structured dts file if this is what you are doing...
>
> I removed the default buffer mode settings that I added into
> 'aspeed-g4.dtsi' and 'aspeed-g5.dtsi' in v1 to avoid touching of the
> default transfer mode setting, but each bus should use its dedicated
> SRAM buffer range for enabling buffer mode so I added this information
> at here as overriding examples instead. I thought that binding document
> is a right place for providing this information but looks like it's not.
> Any recommended place for it? Is it good enough if I add it just into
> the commit message?

I agree with Rob, we don't need this described in the device tree
(binding or dts). We know what the layout is for a given aspeed
family, so the driver can have this information hard coded.

(Correct me if I've misinterpted here Rob)

>
> >> @@ -17,6 +72,25 @@ Optional Properties:
> >>   - bus-frequency    : frequency of the bus clock in Hz defaults to 100 kHz when not
> >>                specified
> >>   - multi-master     : states that there is another master active on this bus.
> >> +- aspeed,dma-buf-size       : size of DMA buffer.
> >> +                        AST2400: N/A
> >> +                        AST2500: 2 ~ 4095
> >> +                        AST2600: 2 ~ 4096
> >
> > If based on the SoC, then all this can be implied from the compatible
> > string.
> >
>
> Please help me to clarify your comment. Should I remove it from here
> with keeping the driver handling code for each SoC compatible string?
> Or should I change it like below?
> aspeed,ast2400-i2c-bus: N/A
> aspeed,ast2500-i2c-bus: 2 ~ 4095
> aspeed,ast2600-i2c-bus: 2 ~ 4096

As above, we know what the buffer size is for the specific soc family,
so we can hard code the value to expect.

The downside of this hard coding is it takes away the option of using
more buffer space for a given master in a system that only enables
some of the masters. Is this a use case you were considering? If so,
then we might revisit some of the advice in this thread.

Cheers,

Joel

Re: [PATCH v2 1/4] dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support

[Jae Hyun Yoo]

Hi Joel

On 1/27/2021 4:06 PM, Joel Stanley wrote:
> On Thu, 14 Jan 2021 at 20:05, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote:
>>
>> Hi Rob,
>>
>> On 1/14/2021 11:34 AM, Rob Herring wrote:
>>>> -- reg                       : address offset and range of bus
>>>> +- reg                       : Address offset and range of bus registers.
>>>> +
>>>> +                      An additional SRAM buffer address offset and range is
>>>> +                      optional in case of enabling I2C dedicated SRAM for
>>>> +                      buffer mode transfer support. If the optional range
>>>> +                      is defined, buffer mode will be enabled.
>>>> +                      - AST2400
>>>> +                        &i2c0 { reg = <0x40 0x40>, <0x800 0x80>; };
>>>> +                        &i2c1 { reg = <0x80 0x40>, <0x880 0x80>; };
>>>> +                        &i2c2 { reg = <0xc0 0x40>, <0x900 0x80>; };
>>>> +                        &i2c3 { reg = <0x100 0x40>, <0x980 0x80>; };
>>>> +                        &i2c4 { reg = <0x140 0x40>, <0xa00 0x80>; };
>>>> +                        &i2c5 { reg = <0x180 0x40>, <0xa80 0x80>; };
>>>> +                        &i2c6 { reg = <0x1c0 0x40>, <0xb00 0x80>; };
>>>> +                        &i2c7 { reg = <0x300 0x40>, <0xb80 0x80>; };
>>>> +                        &i2c8 { reg = <0x340 0x40>, <0xc00 0x80>; };
>>>> +                        &i2c9 { reg = <0x380 0x40>, <0xc80 0x80>; };
>>>> +                        &i2c10 { reg = <0x3c0 0x40>, <0xd00 0x80>; };
>>>> +                        &i2c11 { reg = <0x400 0x40>, <0xd80 0x80>; };
>>>> +                        &i2c12 { reg = <0x440 0x40>, <0xe00 0x80>; };
>>>> +                        &i2c13 { reg = <0x480 0x40>, <0xe80 0x80>; };
>>>
>>> All this information doesn't need to be in the binding.
>>>
>>> It's also an oddly structured dts file if this is what you are doing...
>>
>> I removed the default buffer mode settings that I added into
>> 'aspeed-g4.dtsi' and 'aspeed-g5.dtsi' in v1 to avoid touching of the
>> default transfer mode setting, but each bus should use its dedicated
>> SRAM buffer range for enabling buffer mode so I added this information
>> at here as overriding examples instead. I thought that binding document
>> is a right place for providing this information but looks like it's not.
>> Any recommended place for it? Is it good enough if I add it just into
>> the commit message?
> 
> I agree with Rob, we don't need this described in the device tree
> (binding or dts). We know what the layout is for a given aspeed
> family, so the driver can have this information hard coded.
> 
> (Correct me if I've misinterpted here Rob)
> 

Makes sense. Will add these settings into the driver module as hard
coded per each bus.

>>
>>>> @@ -17,6 +72,25 @@ Optional Properties:
>>>>    - bus-frequency    : frequency of the bus clock in Hz defaults to 100 kHz when not
>>>>                 specified
>>>>    - multi-master     : states that there is another master active on this bus.
>>>> +- aspeed,dma-buf-size       : size of DMA buffer.
>>>> +                        AST2400: N/A
>>>> +                        AST2500: 2 ~ 4095
>>>> +                        AST2600: 2 ~ 4096
>>>
>>> If based on the SoC, then all this can be implied from the compatible
>>> string.
>>>
>>
>> Please help me to clarify your comment. Should I remove it from here
>> with keeping the driver handling code for each SoC compatible string?
>> Or should I change it like below?
>> aspeed,ast2400-i2c-bus: N/A
>> aspeed,ast2500-i2c-bus: 2 ~ 4095
>> aspeed,ast2600-i2c-bus: 2 ~ 4096
> 
> As above, we know what the buffer size is for the specific soc family,
> so we can hard code the value to expect.
> 
> The downside of this hard coding is it takes away the option of using
> more buffer space for a given master in a system that only enables
> some of the masters. Is this a use case you were considering? If so,
> then we might revisit some of the advice in this thread.
> 

I added flexibility on this setting but it doesn't need to be. I'll add
hard coded setting for the maximum DMA length into the driver as you
suggested. If I add a xfer mode setting in device tree instead, enabling
of DMA can be configured as each bus basis so there would be no concern
I believe. Will submit v3 soon.

Thanks,
Jae

Re: [PATCH v2 1/4] dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support

[Jae Hyun Yoo]

Hi Joel

On 1/28/2021 11:36 AM, Jae Hyun Yoo wrote:
> Hi Joel
> 
> On 1/27/2021 4:06 PM, Joel Stanley wrote:
>> On Thu, 14 Jan 2021 at 20:05, Jae Hyun Yoo 
>> <jae.hyun.yoo@linux.intel.com> wrote:
>>>
>>> Hi Rob,
>>>
>>> On 1/14/2021 11:34 AM, Rob Herring wrote:
>>>>> -- reg                       : address offset and range of bus
>>>>> +- reg                       : Address offset and range of bus 
>>>>> registers.
>>>>> +
>>>>> +                      An additional SRAM buffer address offset and 
>>>>> range is
>>>>> +                      optional in case of enabling I2C dedicated 
>>>>> SRAM for
>>>>> +                      buffer mode transfer support. If the 
>>>>> optional range
>>>>> +                      is defined, buffer mode will be enabled.
>>>>> +                      - AST2400
>>>>> +                        &i2c0 { reg = <0x40 0x40>, <0x800 0x80>; };
>>>>> +                        &i2c1 { reg = <0x80 0x40>, <0x880 0x80>; };
>>>>> +                        &i2c2 { reg = <0xc0 0x40>, <0x900 0x80>; };
>>>>> +                        &i2c3 { reg = <0x100 0x40>, <0x980 0x80>; };
>>>>> +                        &i2c4 { reg = <0x140 0x40>, <0xa00 0x80>; };
>>>>> +                        &i2c5 { reg = <0x180 0x40>, <0xa80 0x80>; };
>>>>> +                        &i2c6 { reg = <0x1c0 0x40>, <0xb00 0x80>; };
>>>>> +                        &i2c7 { reg = <0x300 0x40>, <0xb80 0x80>; };
>>>>> +                        &i2c8 { reg = <0x340 0x40>, <0xc00 0x80>; };
>>>>> +                        &i2c9 { reg = <0x380 0x40>, <0xc80 0x80>; };
>>>>> +                        &i2c10 { reg = <0x3c0 0x40>, <0xd00 0x80>; };
>>>>> +                        &i2c11 { reg = <0x400 0x40>, <0xd80 0x80>; };
>>>>> +                        &i2c12 { reg = <0x440 0x40>, <0xe00 0x80>; };
>>>>> +                        &i2c13 { reg = <0x480 0x40>, <0xe80 0x80>; };
>>>>
>>>> All this information doesn't need to be in the binding.
>>>>
>>>> It's also an oddly structured dts file if this is what you are doing...
>>>
>>> I removed the default buffer mode settings that I added into
>>> 'aspeed-g4.dtsi' and 'aspeed-g5.dtsi' in v1 to avoid touching of the
>>> default transfer mode setting, but each bus should use its dedicated
>>> SRAM buffer range for enabling buffer mode so I added this information
>>> at here as overriding examples instead. I thought that binding document
>>> is a right place for providing this information but looks like it's not.
>>> Any recommended place for it? Is it good enough if I add it just into
>>> the commit message?
>>
>> I agree with Rob, we don't need this described in the device tree
>> (binding or dts). We know what the layout is for a given aspeed
>> family, so the driver can have this information hard coded.
>>
>> (Correct me if I've misinterpted here Rob)
>>
> 
> Makes sense. Will add these settings into the driver module as hard
> coded per each bus.
> 

Realized that the SRAM buffer range setting should be added into device
tree because each bus module should get the dedicated IO resource range.
So I'm going to add it to dtsi default reg setting for each I2C bus
and will remove this description in binding. Also, I'll add a mode
setting property instead to keep the current setting as byte mode.

Please let me know if you have any different thought.

Thanks,
Jae

Re: [PATCH v2 1/4] dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support

[Joel Stanley]

On Wed, 3 Feb 2021 at 23:03, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote:
>
> Hi Joel
>
> On 1/28/2021 11:36 AM, Jae Hyun Yoo wrote:
> > Hi Joel
> >
> > On 1/27/2021 4:06 PM, Joel Stanley wrote:
> >>>> All this information doesn't need to be in the binding.
> >>>>
> >>>> It's also an oddly structured dts file if this is what you are doing...
> >>>
> >>> I removed the default buffer mode settings that I added into
> >>> 'aspeed-g4.dtsi' and 'aspeed-g5.dtsi' in v1 to avoid touching of the
> >>> default transfer mode setting, but each bus should use its dedicated
> >>> SRAM buffer range for enabling buffer mode so I added this information
> >>> at here as overriding examples instead. I thought that binding document
> >>> is a right place for providing this information but looks like it's not.
> >>> Any recommended place for it? Is it good enough if I add it just into
> >>> the commit message?
> >>
> >> I agree with Rob, we don't need this described in the device tree
> >> (binding or dts). We know what the layout is for a given aspeed
> >> family, so the driver can have this information hard coded.
> >>
> >> (Correct me if I've misinterpted here Rob)
> >>
> >
> > Makes sense. Will add these settings into the driver module as hard
> > coded per each bus.
> >
>
> Realized that the SRAM buffer range setting should be added into device
> tree because each bus module should get the dedicated IO resource range.
> So I'm going to add it to dtsi default reg setting for each I2C bus
> and will remove this description in binding. Also, I'll add a mode
> setting property instead to keep the current setting as byte mode.

I don't understand. What do you propose adding?

Re: [PATCH v2 1/4] dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support

[Jae Hyun Yoo]

On 2/9/2021 4:10 AM, Joel Stanley wrote:
> On Wed, 3 Feb 2021 at 23:03, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote:
>>
>> Hi Joel
>>
>> On 1/28/2021 11:36 AM, Jae Hyun Yoo wrote:
>>> Hi Joel
>>>
>>> On 1/27/2021 4:06 PM, Joel Stanley wrote:
>>>>>> All this information doesn't need to be in the binding.
>>>>>>
>>>>>> It's also an oddly structured dts file if this is what you are doing...
>>>>>
>>>>> I removed the default buffer mode settings that I added into
>>>>> 'aspeed-g4.dtsi' and 'aspeed-g5.dtsi' in v1 to avoid touching of the
>>>>> default transfer mode setting, but each bus should use its dedicated
>>>>> SRAM buffer range for enabling buffer mode so I added this information
>>>>> at here as overriding examples instead. I thought that binding document
>>>>> is a right place for providing this information but looks like it's not.
>>>>> Any recommended place for it? Is it good enough if I add it just into
>>>>> the commit message?
>>>>
>>>> I agree with Rob, we don't need this described in the device tree
>>>> (binding or dts). We know what the layout is for a given aspeed
>>>> family, so the driver can have this information hard coded.
>>>>
>>>> (Correct me if I've misinterpted here Rob)
>>>>
>>>
>>> Makes sense. Will add these settings into the driver module as hard
>>> coded per each bus.
>>>
>>
>> Realized that the SRAM buffer range setting should be added into device
>> tree because each bus module should get the dedicated IO resource range.
>> So I'm going to add it to dtsi default reg setting for each I2C bus
>> and will remove this description in binding. Also, I'll add a mode
>> setting property instead to keep the current setting as byte mode.
> 
> I don't understand. What do you propose adding?
> 

I'm going to add reg resource for the SRAM buffer per each bus like

reg = <0x40 0x40>, <0x800 0x80>;

into the aspeed-g*.dtsi but adding like this will not be a key for
enabling buffer mode like this v2 does. Also, I'm going to remove the
'aspeed,dma-buf-size' in this patch series and I'll add fixed DMA length
as a configuration per each SoC. Instead, I'm going to add a xfer mode
property e.g. 'aspeed,i2c-xfer-mode' to select 'byte', 'buffer' or 'dma'
modes.

Thanks,
Jae