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

[PATCH v4 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, buffer mode and DMA mode can be
selectively used depend 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 v3:
- Added a comment to explain SRAM buffer handling logic.

Changes since v2:
- Added SRAM resources back to default dtsi and added mode selection
  property.
- Refined SoC family dependent xfer mode configuration functions.

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 transfer mode 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    |  37 +-
 arch/arm/boot/dts/aspeed-g4.dtsi              |  47 +-
 arch/arm/boot/dts/aspeed-g5.dtsi              |  47 +-
 arch/arm/boot/dts/aspeed-g6.dtsi              |  32 +-
 drivers/i2c/busses/i2c-aspeed.c               | 641 ++++++++++++++++--
 5 files changed, 688 insertions(+), 116 deletions(-)

-- 
2.17.1

[PATCH v4 1/4] dt-bindings: i2c: aspeed: add transfer mode support

[Jae Hyun Yoo]

Append bindings to support transfer mode.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
---
Changes since v3:
- None

Changes since v2:
- Moved SRAM resources back to default dtsi and added mode selection
  property.

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    | 37 +++++++++++++++----
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
index b47f6ccb196a..242343177324 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
@@ -17,6 +17,20 @@ 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,i2c-xfer-mode	: should be "byte", "buf" or "dma" to select transfer
+			  mode defaults to "byte" mode when not specified.
+
+			  I2C DMA mode on 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,20 +40,29 @@ 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 {
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x40 0x40>;
-		compatible = "aspeed,ast2400-i2c-bus";
+		reg = <0x40 0x40>, <0x200 0x10>;
+		compatible = "aspeed,ast2500-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
 		bus-frequency = <100000>;
-- 
2.17.1

[PATCH v4 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>
Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
---
Changes since v3:
- None

Changes since v2:
- Added SRAM resources back to default dtsi and added mode selection
  property.

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 | 47 +++++++++++++++++++-------------
 arch/arm/boot/dts/aspeed-g5.dtsi | 47 +++++++++++++++++++-------------
 arch/arm/boot/dts/aspeed-g6.dtsi | 32 +++++++++++-----------
 3 files changed, 72 insertions(+), 54 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index e7a45ba18fc9..16d13c2003e5 100644
--- a/arch/arm/boot/dts/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed-g4.dtsi
@@ -443,12 +443,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 {
@@ -456,7 +465,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x40 0x40>;
+		reg = <0x40 0x40>, <0x800 0x80>;
 		compatible = "aspeed,ast2400-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -472,7 +481,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x80 0x40>;
+		reg = <0x80 0x40>, <0x880 0x80>;
 		compatible = "aspeed,ast2400-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -488,7 +497,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0xc0 0x40>;
+		reg = <0xc0 0x40>, <0x900 0x80>;
 		compatible = "aspeed,ast2400-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -505,7 +514,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x100 0x40>;
+		reg = <0x100 0x40>, <0x980 0x80>;
 		compatible = "aspeed,ast2400-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -522,7 +531,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x140 0x40>;
+		reg = <0x140 0x40>, <0xa00 0x80>;
 		compatible = "aspeed,ast2400-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -539,7 +548,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x180 0x40>;
+		reg = <0x180 0x40>, <0xa80 0x80>;
 		compatible = "aspeed,ast2400-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -556,7 +565,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x1c0 0x40>;
+		reg = <0x1c0 0x40>, <0xb00 0x80>;
 		compatible = "aspeed,ast2400-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -573,7 +582,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x300 0x40>;
+		reg = <0x300 0x40>, <0xb80 0x80>;
 		compatible = "aspeed,ast2400-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -590,7 +599,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x340 0x40>;
+		reg = <0x340 0x40>, <0xc00 0x80>;
 		compatible = "aspeed,ast2400-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -607,7 +616,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x380 0x40>;
+		reg = <0x380 0x40>, <0xc80 0x80>;
 		compatible = "aspeed,ast2400-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -624,7 +633,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x3c0 0x40>;
+		reg = <0x3c0 0x40>, <0xd00 0x80>;
 		compatible = "aspeed,ast2400-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -641,7 +650,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x400 0x40>;
+		reg = <0x400 0x40>, <0xd80 0x80>;
 		compatible = "aspeed,ast2400-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -658,7 +667,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x440 0x40>;
+		reg = <0x440 0x40>, <0xe00 0x80>;
 		compatible = "aspeed,ast2400-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -675,7 +684,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x480 0x40>;
+		reg = <0x480 0x40>, <0xe80 0x80>;
 		compatible = "aspeed,ast2400-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index 21930521a986..b97041559474 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -566,12 +566,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 {
@@ -579,7 +588,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x40 0x40>;
+		reg = <0x40 0x40>, <0x200 0x10>;
 		compatible = "aspeed,ast2500-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -595,7 +604,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x80 0x40>;
+		reg = <0x80 0x40>, <0x210 0x10>;
 		compatible = "aspeed,ast2500-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -611,7 +620,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0xc0 0x40>;
+		reg = <0xc0 0x40>, <0x220 0x10>;
 		compatible = "aspeed,ast2500-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -628,7 +637,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x100 0x40>;
+		reg = <0x100 0x40>, <0x230 0x10>;
 		compatible = "aspeed,ast2500-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -645,7 +654,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x140 0x40>;
+		reg = <0x140 0x40>, <0x240 0x10>;
 		compatible = "aspeed,ast2500-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -662,7 +671,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x180 0x40>;
+		reg = <0x180 0x40>, <0x250 0x10>;
 		compatible = "aspeed,ast2500-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -679,7 +688,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x1c0 0x40>;
+		reg = <0x1c0 0x40>, <0x260 0x10>;
 		compatible = "aspeed,ast2500-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -696,7 +705,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x300 0x40>;
+		reg = <0x300 0x40>, <0x270 0x10>;
 		compatible = "aspeed,ast2500-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -713,7 +722,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x340 0x40>;
+		reg = <0x340 0x40>, <0x280 0x10>;
 		compatible = "aspeed,ast2500-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -730,7 +739,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x380 0x40>;
+		reg = <0x380 0x40>, <0x290 0x10>;
 		compatible = "aspeed,ast2500-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -747,7 +756,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x3c0 0x40>;
+		reg = <0x3c0 0x40>, <0x2a0 0x10>;
 		compatible = "aspeed,ast2500-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -764,7 +773,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x400 0x40>;
+		reg = <0x400 0x40>, <0x2b0 0x10>;
 		compatible = "aspeed,ast2500-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -781,7 +790,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x440 0x40>;
+		reg = <0x440 0x40>, <0x2c0 0x10>;
 		compatible = "aspeed,ast2500-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -798,7 +807,7 @@
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
 
-		reg = <0x480 0x40>;
+		reg = <0x480 0x40>, <0x2d0 0x10>;
 		compatible = "aspeed,ast2500-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB>;
 		resets = <&syscon ASPEED_RESET_I2C>;
diff --git a/arch/arm/boot/dts/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed-g6.dtsi
index 3ee470c2b7b5..48f86f4587db 100644
--- a/arch/arm/boot/dts/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed-g6.dtsi
@@ -695,7 +695,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x80 0x80>;
+		reg = <0x80 0x80>, <0xc00 0x20>;
 		compatible = "aspeed,ast2600-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB2>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -710,7 +710,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x100 0x80>;
+		reg = <0x100 0x80>, <0xc20 0x20>;
 		compatible = "aspeed,ast2600-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB2>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -725,7 +725,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x180 0x80>;
+		reg = <0x180 0x80>, <0xc40 0x20>;
 		compatible = "aspeed,ast2600-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB2>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -740,7 +740,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x200 0x80>;
+		reg = <0x200 0x80>, <0xc60 0x20>;
 		compatible = "aspeed,ast2600-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB2>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -755,7 +755,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x280 0x80>;
+		reg = <0x280 0x80>, <0xc80 0x20>;
 		compatible = "aspeed,ast2600-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB2>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -770,7 +770,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x300 0x80>;
+		reg = <0x300 0x80>, <0xca0 0x20>;
 		compatible = "aspeed,ast2600-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB2>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -785,7 +785,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x380 0x80>;
+		reg = <0x380 0x80>, <0xcc0 0x20>;
 		compatible = "aspeed,ast2600-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB2>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -800,7 +800,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x400 0x80>;
+		reg = <0x400 0x80>, <0xce0 0x20>;
 		compatible = "aspeed,ast2600-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB2>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -815,7 +815,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x480 0x80>;
+		reg = <0x480 0x80>, <0xd00 0x20>;
 		compatible = "aspeed,ast2600-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB2>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -830,7 +830,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x500 0x80>;
+		reg = <0x500 0x80>, <0xd20 0x20>;
 		compatible = "aspeed,ast2600-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB2>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -845,7 +845,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x580 0x80>;
+		reg = <0x580 0x80>, <0xd40 0x20>;
 		compatible = "aspeed,ast2600-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB2>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -860,7 +860,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x600 0x80>;
+		reg = <0x600 0x80>, <0xd60 0x20>;
 		compatible = "aspeed,ast2600-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB2>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -875,7 +875,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x680 0x80>;
+		reg = <0x680 0x80>, <0xd80 0x20>;
 		compatible = "aspeed,ast2600-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB2>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -890,7 +890,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x700 0x80>;
+		reg = <0x700 0x80>, <0xda0 0x20>;
 		compatible = "aspeed,ast2600-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB2>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -905,7 +905,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x780 0x80>;
+		reg = <0x780 0x80>, <0xdc0 0x20>;
 		compatible = "aspeed,ast2600-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB2>;
 		resets = <&syscon ASPEED_RESET_I2C>;
@@ -920,7 +920,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 		#interrupt-cells = <1>;
-		reg = <0x800 0x80>;
+		reg = <0x800 0x80>, <0xde0 0x20>;
 		compatible = "aspeed,ast2600-i2c-bus";
 		clocks = <&syscon ASPEED_CLK_APB2>;
 		resets = <&syscon ASPEED_RESET_I2C>;
-- 
2.17.1

[PATCH v4 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 v3:
- Added a comment to explain SRAM buffer handling logic.

Changes since v2:
- Refined SoC family dependent xfer mode configuration functions.

Changes since v1:
- Updated commit message.
- Refined using abstract functions.

 drivers/i2c/busses/i2c-aspeed.c | 468 ++++++++++++++++++++++++++++----
 1 file changed, 416 insertions(+), 52 deletions(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 724bf30600d6..ffc52937df26 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,
@@ -140,6 +157,12 @@ enum aspeed_i2c_slave_state {
 	ASPEED_I2C_SLAVE_STOP,
 };
 
+struct aspeed_i2c_config {
+	u32 (*get_clk_reg_val)(struct device *dev, u32 divisor);
+	int (*enable_sram)(void);
+	int (*set_buf_xfer_mode)(struct device *dev);
+};
+
 struct aspeed_i2c_bus {
 	struct i2c_adapter		adap;
 	struct device			*dev;
@@ -148,8 +171,7 @@ struct aspeed_i2c_bus {
 	/* Synchronizes I/O mem access to base. */
 	spinlock_t			lock;
 	struct completion		cmd_complete;
-	u32				(*get_clk_reg_val)(struct device *dev,
-							   u32 divisor);
+	struct aspeed_i2c_config	*config;
 	unsigned long			parent_clk_frequency;
 	u32				bus_frequency;
 	/* Transaction state. */
@@ -164,6 +186,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 +268,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 +365,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 +380,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 +414,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 +438,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 +526,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 +575,107 @@ 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;
+
+		/*
+		 * 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 - (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 +784,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 +804,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);
 		}
@@ -887,7 +1162,7 @@ static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
 	clk_reg_val &= (ASPEED_I2CD_TIME_TBUF_MASK |
 			ASPEED_I2CD_TIME_THDSTA_MASK |
 			ASPEED_I2CD_TIME_TACST_MASK);
-	clk_reg_val |= bus->get_clk_reg_val(bus->dev, divisor);
+	clk_reg_val |= bus->config->get_clk_reg_val(bus->dev, divisor);
 	writel(clk_reg_val, bus->base + ASPEED_I2C_AC_TIMING_REG1);
 	writel(ASPEED_NO_TIMEOUT_CTRL, bus->base + ASPEED_I2C_AC_TIMING_REG2);
 
@@ -908,6 +1183,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,40 +1226,129 @@ static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)
 	return ret;
 }
 
+static int aspeed_i2c_24xx_enable_sram(void)
+{
+	/* SRAM is enabled by default */
+
+	return 0;
+}
+
+static int aspeed_i2c_25xx_enable_sram(void)
+{
+	struct regmap *gr_regmap;
+	int ret;
+
+	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);
+
+	return ret;
+}
+
+static int aspeed_i2c_24xx_set_buf_xfer_mode(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct aspeed_i2c_bus *bus = platform_get_drvdata(pdev);
+	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);
+		bus->buf_page = ((res->start >> 8) & GENMASK(3, 0)) - 8;
+		bus->buf_offset = (res->start >> 2) &
+				  ASPEED_I2CD_BUF_OFFSET_MASK;
+	}
+
+	return bus->buf_size ? 0 : -EINVAL;
+}
+
+static int aspeed_i2c_25xx_set_buf_xfer_mode(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct aspeed_i2c_bus *bus = platform_get_drvdata(pdev);
+	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);
+
+	return bus->buf_size ? 0 : -EINVAL;
+}
+
+static const struct aspeed_i2c_config ast24xx_config = {
+	.get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val,
+	.enable_sram = aspeed_i2c_24xx_enable_sram,
+	.set_buf_xfer_mode = aspeed_i2c_24xx_set_buf_xfer_mode,
+};
+
+static const struct aspeed_i2c_config ast25xx_config = {
+	.get_clk_reg_val = aspeed_i2c_25xx_get_clk_reg_val,
+	.enable_sram = aspeed_i2c_25xx_enable_sram,
+	.set_buf_xfer_mode = aspeed_i2c_25xx_set_buf_xfer_mode,
+};
+
+static const struct aspeed_i2c_config ast26xx_config = {
+	.get_clk_reg_val = aspeed_i2c_25xx_get_clk_reg_val,
+	.enable_sram = aspeed_i2c_24xx_enable_sram,
+	.set_buf_xfer_mode = aspeed_i2c_25xx_set_buf_xfer_mode,
+};
+
 static const struct of_device_id aspeed_i2c_bus_of_table[] = {
-	{
-		.compatible = "aspeed,ast2400-i2c-bus",
-		.data = aspeed_i2c_24xx_get_clk_reg_val,
-	},
-	{
-		.compatible = "aspeed,ast2500-i2c-bus",
-		.data = aspeed_i2c_25xx_get_clk_reg_val,
-	},
-	{
-		.compatible = "aspeed,ast2600-i2c-bus",
-		.data = aspeed_i2c_25xx_get_clk_reg_val,
-	},
-	{ },
+	{ .compatible = "aspeed,ast2400-i2c-bus", .data = &ast24xx_config, },
+	{ .compatible = "aspeed,ast2500-i2c-bus", .data = &ast25xx_config, },
+	{ .compatible = "aspeed,ast2600-i2c-bus", .data = &ast26xx_config, },
+	{}
 };
 MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table);
 
+static void aspeed_i2c_set_xfer_mode(struct aspeed_i2c_bus *bus)
+{
+	struct platform_device *pdev = to_platform_device(bus->dev);
+	const char *mode;
+	int ret;
+
+	mode = of_get_property(pdev->dev.of_node, "aspeed,i2c-xfer-mode", NULL);
+	if (!mode || strncasecmp(mode, "byte", 4) == 0)
+		return;
+
+	ret = bus->config->enable_sram();
+	if (!ret && !strncasecmp(mode, "buf", 3))
+		ret = bus->config->set_buf_xfer_mode(bus->dev);
+
+	if (ret)
+		dev_dbg(&pdev->dev, "Use default (byte) xfer mode\n");
+}
+
 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;
+	platform_set_drvdata(pdev, bus);
+
 	parent_clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(parent_clk))
 		return PTR_ERR(parent_clk);
@@ -1007,10 +1374,11 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
 
 	match = of_match_node(aspeed_i2c_bus_of_table, pdev->dev.of_node);
 	if (!match)
-		bus->get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val;
-	else
-		bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
-				match->data;
+		return -EINVAL;
+
+	bus->config = (struct aspeed_i2c_config *)match->data;
+
+	aspeed_i2c_set_xfer_mode(bus);
 
 	/* Initialize the I2C adapter */
 	spin_lock_init(&bus->lock);
@@ -1023,8 +1391,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);
@@ -1046,10 +1412,8 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	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 ? "buf" : "byte", irq);
 
 	return 0;
 }
-- 
2.17.1

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

[Jae Hyun Yoo]

This commit adds DMA mode transfer support.

Only AST2500 and later versions support 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 v3:
- None

Changes since v2:
- Refined SoC family dependent xfer mode configuration functions.

Changes since v1:
- Updated commit message and comments.
- Refined using abstract functions.

 drivers/i2c/busses/i2c-aspeed.c | 265 ++++++++++++++++++++++++++------
 1 file changed, 216 insertions(+), 49 deletions(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index ffc52937df26..3e3bb014b027 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,
@@ -161,6 +175,7 @@ struct aspeed_i2c_config {
 	u32 (*get_clk_reg_val)(struct device *dev, u32 divisor);
 	int (*enable_sram)(void);
 	int (*set_buf_xfer_mode)(struct device *dev);
+	int (*set_dma_xfer_mode)(struct device *dev);
 };
 
 struct aspeed_i2c_bus {
@@ -190,6 +205,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;
@@ -272,9 +293,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;
@@ -288,7 +313,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));
@@ -305,7 +341,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,
@@ -321,7 +364,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));
@@ -451,7 +510,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) |
@@ -474,7 +541,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;
 
@@ -527,18 +605,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);
@@ -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 {
@@ -633,7 +725,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);
@@ -650,7 +749,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) {
@@ -660,14 +759,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;
@@ -1287,22 +1396,63 @@ static int aspeed_i2c_25xx_set_buf_xfer_mode(struct device *dev)
 	return bus->buf_size ? 0 : -EINVAL;
 }
 
+static int aspeed_i2c_24xx_set_dma_xfer_mode(struct device *dev)
+{
+	/* AST24xx doesn't support DMA mode */
+
+	return -EBADR;
+}
+
+static int aspeed_i2c_25xx_set_dma_xfer_mode(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct aspeed_i2c_bus *bus = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
+	if (!ret) {
+		bus->buf_size = ASPEED_I2CD_DMA_LEN_MASK >>
+				ASPEED_I2CD_DMA_LEN_SHIFT;
+		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) {
+			ret = -ENOMEM;
+			bus->buf_size = 0;
+			dev_dbg(&pdev->dev, "Cannot allocate DMA buffer\n");
+			dma_pool_destroy(bus->dma_pool);
+		}
+	}
+
+	return ret;
+}
+
 static const struct aspeed_i2c_config ast24xx_config = {
 	.get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val,
 	.enable_sram = aspeed_i2c_24xx_enable_sram,
 	.set_buf_xfer_mode = aspeed_i2c_24xx_set_buf_xfer_mode,
+	.set_dma_xfer_mode = aspeed_i2c_24xx_set_dma_xfer_mode,
 };
 
 static const struct aspeed_i2c_config ast25xx_config = {
 	.get_clk_reg_val = aspeed_i2c_25xx_get_clk_reg_val,
 	.enable_sram = aspeed_i2c_25xx_enable_sram,
 	.set_buf_xfer_mode = aspeed_i2c_25xx_set_buf_xfer_mode,
+	.set_dma_xfer_mode = aspeed_i2c_25xx_set_dma_xfer_mode,
 };
 
 static const struct aspeed_i2c_config ast26xx_config = {
 	.get_clk_reg_val = aspeed_i2c_25xx_get_clk_reg_val,
 	.enable_sram = aspeed_i2c_24xx_enable_sram,
 	.set_buf_xfer_mode = aspeed_i2c_25xx_set_buf_xfer_mode,
+	.set_dma_xfer_mode = aspeed_i2c_25xx_set_dma_xfer_mode,
 };
 
 static const struct of_device_id aspeed_i2c_bus_of_table[] = {
@@ -1324,8 +1474,12 @@ static void aspeed_i2c_set_xfer_mode(struct aspeed_i2c_bus *bus)
 		return;
 
 	ret = bus->config->enable_sram();
-	if (!ret && !strncasecmp(mode, "buf", 3))
-		ret = bus->config->set_buf_xfer_mode(bus->dev);
+	if (!ret) {
+		if (!strncasecmp(mode, "buf", 3))
+			ret = bus->config->set_buf_xfer_mode(bus->dev);
+		else if (!strncasecmp(mode, "dma", 3))
+			ret = bus->config->set_dma_xfer_mode(bus->dev);
+	}
 
 	if (ret)
 		dev_dbg(&pdev->dev, "Use default (byte) xfer mode\n");
@@ -1400,22 +1554,31 @@ 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;
 
 	dev_info(bus->dev, "i2c bus %d registered (%s mode), irq %d\n",
-		 bus->adap.nr, bus->buf_base ? "buf" : "byte", irq);
+		 bus->adap.nr, bus->dma_buf ? "dma" :
+					      bus->buf_base ? "buf" : "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)
@@ -1433,6 +1596,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 v4 1/4] dt-bindings: i2c: aspeed: add transfer mode support

[Rob Herring]

On Wed, Feb 24, 2021 at 11:17:17AM -0800, Jae Hyun Yoo wrote:
> Append bindings to support transfer mode.
> 
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> ---
> Changes since v3:
> - None
> 
> Changes since v2:
> - Moved SRAM resources back to default dtsi and added mode selection
>   property.
> 
> 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    | 37 +++++++++++++++----
>  1 file changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> index b47f6ccb196a..242343177324 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> @@ -17,6 +17,20 @@ 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,i2c-xfer-mode	: should be "byte", "buf" or "dma" to select transfer
> +			  mode defaults to "byte" mode when not specified.
> +
> +			  I2C DMA mode on 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

How does one decide between byte or buf mode? 

>  
>  Example:
>  
> @@ -26,20 +40,29 @@ 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 {
>  		#address-cells = <1>;
>  		#size-cells = <0>;
>  		#interrupt-cells = <1>;
> -		reg = <0x40 0x40>;
> -		compatible = "aspeed,ast2400-i2c-bus";
> +		reg = <0x40 0x40>, <0x200 0x10>;
> +		compatible = "aspeed,ast2500-i2c-bus";


The example changes are all unrelated to adding the new property. Should 
be a separate patch or just dropped.

>  		clocks = <&syscon ASPEED_CLK_APB>;
>  		resets = <&syscon ASPEED_RESET_I2C>;
>  		bus-frequency = <100000>;
> -- 
> 2.17.1
> 

Re: [PATCH v4 1/4] dt-bindings: i2c: aspeed: add transfer mode support

[Jae Hyun Yoo]

Hi Rob,

On 3/6/2021 12:30 PM, Rob Herring wrote:
> On Wed, Feb 24, 2021 at 11:17:17AM -0800, Jae Hyun Yoo wrote:
>> Append bindings to support transfer mode.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>> ---
>> Changes since v3:
>> - None
>>
>> Changes since v2:
>> - Moved SRAM resources back to default dtsi and added mode selection
>>    property.
>>
>> 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    | 37 +++++++++++++++----
>>   1 file changed, 30 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>> index b47f6ccb196a..242343177324 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>> @@ -17,6 +17,20 @@ 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,i2c-xfer-mode	: should be "byte", "buf" or "dma" to select transfer
>> +			  mode defaults to "byte" mode when not specified.
>> +
>> +			  I2C DMA mode on 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
> 
> How does one decide between byte or buf mode?

If a given system makes just one byte r/w transactions most of the time
then byte mode will be a right setting. Otherwise, buf mode is more
efficient because it doesn't generate a bunch of interrupts on every
byte handling.

>>   
>>   Example:
>>   
>> @@ -26,20 +40,29 @@ 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 {
>>   		#address-cells = <1>;
>>   		#size-cells = <0>;
>>   		#interrupt-cells = <1>;
>> -		reg = <0x40 0x40>;
>> -		compatible = "aspeed,ast2400-i2c-bus";
>> +		reg = <0x40 0x40>, <0x200 0x10>;
>> +		compatible = "aspeed,ast2500-i2c-bus";
> 
> The example changes are all unrelated to adding the new property. Should
> be a separate patch or just dropped.

The example changes are not directly related to the new property but
related to the transfer mode support in this patch set. 'i2c_gr' node is
added to provide a way for accessing I2C global registers to enable
I2C SRAM, and 'reg' is modified to add the SRAM resource range.

Thanks,
Jae

>>   		clocks = <&syscon ASPEED_CLK_APB>;
>>   		resets = <&syscon ASPEED_RESET_I2C>;
>>   		bus-frequency = <100000>;
>> -- 
>> 2.17.1
>>

Re: [PATCH v4 1/4] dt-bindings: i2c: aspeed: add transfer mode support

[Rob Herring]

On Tue, Mar 9, 2021 at 10:02 AM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> wrote:
>
> Hi Rob,
>
> On 3/6/2021 12:30 PM, Rob Herring wrote:
> > On Wed, Feb 24, 2021 at 11:17:17AM -0800, Jae Hyun Yoo wrote:
> >> Append bindings to support transfer mode.
> >>
> >> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> >> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> >> ---
> >> Changes since v3:
> >> - None
> >>
> >> Changes since v2:
> >> - Moved SRAM resources back to default dtsi and added mode selection
> >>    property.
> >>
> >> 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    | 37 +++++++++++++++----
> >>   1 file changed, 30 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> >> index b47f6ccb196a..242343177324 100644
> >> --- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> >> +++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> >> @@ -17,6 +17,20 @@ 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,i2c-xfer-mode      : should be "byte", "buf" or "dma" to select transfer
> >> +                      mode defaults to "byte" mode when not specified.
> >> +
> >> +                      I2C DMA mode on 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
> >
> > How does one decide between byte or buf mode?
>
> If a given system makes just one byte r/w transactions most of the time
> then byte mode will be a right setting. Otherwise, buf mode is more
> efficient because it doesn't generate a bunch of interrupts on every
> byte handling.

Then why doesn't the driver do byte transactions when it gets small
1-4? byte transactions and buffer transactions when it gets larger
sized transactions.

Rob

Re: [PATCH v4 1/4] dt-bindings: i2c: aspeed: add transfer mode support

[Jae Hyun Yoo]

On 3/9/2021 6:15 PM, Rob Herring wrote:
> On Tue, Mar 9, 2021 at 10:02 AM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
>>
>> Hi Rob,
>>
>> On 3/6/2021 12:30 PM, Rob Herring wrote:
>>> On Wed, Feb 24, 2021 at 11:17:17AM -0800, Jae Hyun Yoo wrote:
>>>> Append bindings to support transfer mode.
>>>>
>>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>>>> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>>>> ---
>>>> Changes since v3:
>>>> - None
>>>>
>>>> Changes since v2:
>>>> - Moved SRAM resources back to default dtsi and added mode selection
>>>>     property.
>>>>
>>>> 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    | 37 +++++++++++++++----
>>>>    1 file changed, 30 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>>>> index b47f6ccb196a..242343177324 100644
>>>> --- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>>>> @@ -17,6 +17,20 @@ 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,i2c-xfer-mode      : should be "byte", "buf" or "dma" to select transfer
>>>> +                      mode defaults to "byte" mode when not specified.
>>>> +
>>>> +                      I2C DMA mode on 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
>>>
>>> How does one decide between byte or buf mode?
>>
>> If a given system makes just one byte r/w transactions most of the time
>> then byte mode will be a right setting. Otherwise, buf mode is more
>> efficient because it doesn't generate a bunch of interrupts on every
>> byte handling.
> 
> Then why doesn't the driver do byte transactions when it gets small
> 1-4? byte transactions and buffer transactions when it gets larger
> sized transactions.

Good question and it could be an option of this implementation.
Actually, each mode needs different register handling so we need to add
additional conditional branches to make it dynamic mode change depends
on the data size which can be a downside. Also, checked that small
amount of data transfer efficiency in 'buf' transfer mode is almost same
to 'byte' mode so there would be no big benefit from the dynamic mode
change. Of course, we can remove the 'byte' transfer mode but we should
also provide flexibility of configuration on what this hardware can
support, IMO.

Thanks,
Jae

Re: [PATCH v4 1/4] dt-bindings: i2c: aspeed: add transfer mode support

[Jae Hyun Yoo]

Ping.

On 3/10/2021 7:55 AM, Jae Hyun Yoo wrote:
> On 3/9/2021 6:15 PM, Rob Herring wrote:
>> On Tue, Mar 9, 2021 at 10:02 AM Jae Hyun Yoo
>> <jae.hyun.yoo@linux.intel.com> wrote:
>>>
>>> Hi Rob,
>>>
>>> On 3/6/2021 12:30 PM, Rob Herring wrote:
>>>> On Wed, Feb 24, 2021 at 11:17:17AM -0800, Jae Hyun Yoo wrote:
>>>>> Append bindings to support transfer mode.
>>>>>
>>>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>>>>> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>>>>> ---
>>>>> Changes since v3:
>>>>> - None
>>>>>
>>>>> Changes since v2:
>>>>> - Moved SRAM resources back to default dtsi and added mode selection
>>>>>     property.
>>>>>
>>>>> 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    | 37 
>>>>> +++++++++++++++----
>>>>>    1 file changed, 30 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt 
>>>>> b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>>>>> index b47f6ccb196a..242343177324 100644
>>>>> --- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>>>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>>>>> @@ -17,6 +17,20 @@ 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,i2c-xfer-mode      : should be "byte", "buf" or "dma" to 
>>>>> select transfer
>>>>> +                      mode defaults to "byte" mode when not 
>>>>> specified.
>>>>> +
>>>>> +                      I2C DMA mode on 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
>>>>
>>>> How does one decide between byte or buf mode?
>>>
>>> If a given system makes just one byte r/w transactions most of the time
>>> then byte mode will be a right setting. Otherwise, buf mode is more
>>> efficient because it doesn't generate a bunch of interrupts on every
>>> byte handling.
>>
>> Then why doesn't the driver do byte transactions when it gets small
>> 1-4? byte transactions and buffer transactions when it gets larger
>> sized transactions.
> 
> Good question and it could be an option of this implementation.
> Actually, each mode needs different register handling so we need to add
> additional conditional branches to make it dynamic mode change depends
> on the data size which can be a downside. Also, checked that small
> amount of data transfer efficiency in 'buf' transfer mode is almost same
> to 'byte' mode so there would be no big benefit from the dynamic mode
> change. Of course, we can remove the 'byte' transfer mode but we should
> also provide flexibility of configuration on what this hardware can
> support, IMO.
> 
> Thanks,
> Jae

Re: [PATCH v4 1/4] dt-bindings: i2c: aspeed: add transfer mode support

[Brendan Higgins]

On Thu, Apr 8, 2021 at 10:50 AM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> wrote:
>
> Ping.
>
> On 3/10/2021 7:55 AM, Jae Hyun Yoo wrote:
> > On 3/9/2021 6:15 PM, Rob Herring wrote:
> >> On Tue, Mar 9, 2021 at 10:02 AM Jae Hyun Yoo
> >> <jae.hyun.yoo@linux.intel.com> wrote:
> >>>
> >>> Hi Rob,
> >>>
> >>> On 3/6/2021 12:30 PM, Rob Herring wrote:
> >>>> On Wed, Feb 24, 2021 at 11:17:17AM -0800, Jae Hyun Yoo wrote:
> >>>>> Append bindings to support transfer mode.
> >>>>>
> >>>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> >>>>> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> >>>>> ---
> >>>>> Changes since v3:
> >>>>> - None
> >>>>>
> >>>>> Changes since v2:
> >>>>> - Moved SRAM resources back to default dtsi and added mode selection
> >>>>>     property.
> >>>>>
> >>>>> 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    | 37
> >>>>> +++++++++++++++----
> >>>>>    1 file changed, 30 insertions(+), 7 deletions(-)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> >>>>> b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> >>>>> index b47f6ccb196a..242343177324 100644
> >>>>> --- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> >>>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> >>>>> @@ -17,6 +17,20 @@ 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,i2c-xfer-mode      : should be "byte", "buf" or "dma" to
> >>>>> select transfer
> >>>>> +                      mode defaults to "byte" mode when not
> >>>>> specified.
> >>>>> +
> >>>>> +                      I2C DMA mode on 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
> >>>>
> >>>> How does one decide between byte or buf mode?
> >>>
> >>> If a given system makes just one byte r/w transactions most of the time
> >>> then byte mode will be a right setting. Otherwise, buf mode is more
> >>> efficient because it doesn't generate a bunch of interrupts on every
> >>> byte handling.
> >>
> >> Then why doesn't the driver do byte transactions when it gets small
> >> 1-4? byte transactions and buffer transactions when it gets larger
> >> sized transactions.
> >
> > Good question and it could be an option of this implementation.
> > Actually, each mode needs different register handling so we need to add
> > additional conditional branches to make it dynamic mode change depends
> > on the data size which can be a downside. Also, checked that small
> > amount of data transfer efficiency in 'buf' transfer mode is almost same
> > to 'byte' mode so there would be no big benefit from the dynamic mode
> > change. Of course, we can remove the 'byte' transfer mode but we should
> > also provide flexibility of configuration on what this hardware can
> > support, IMO.

I would rather set the choice in device tree or Kconfig, which the
former is what I think you did here.

As for doing byte mode for small transactions and buffer/DMA for large
transactions, I would prefer sticking to a single mode based on what
is selected at build/boot time. Seems less error prone to me. Then
again, Rob probably has more experience in this area than I do, so
maybe this kind of thing is pretty common and I just don't realize it.

In any case, as for getting rid of byte mode, I would support that,
but not in this patch set. I would rather switch the default and get
users on buffer/DMA mode before taking away a fallback option.

My 2 cents, but I think the OzLabs and other active OpenBMC people are
probably a little more up to date on this.

Cheers

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

[Brendan Higgins]

On Wed, Feb 24, 2021 at 11:04 AM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> wrote:
>
> 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>

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

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

[Brendan Higgins]

On Wed, Feb 24, 2021 at 11:04 AM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> wrote:
>
> This commit adds DMA mode transfer support.
>
> Only AST2500 and later versions support 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 v3:
> - None
>
> Changes since v2:
> - Refined SoC family dependent xfer mode configuration functions.
>
> Changes since v1:
> - Updated commit message and comments.
> - Refined using abstract functions.
>
>  drivers/i2c/busses/i2c-aspeed.c | 265 ++++++++++++++++++++++++++------
>  1 file changed, 216 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index ffc52937df26..3e3bb014b027 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,
> @@ -161,6 +175,7 @@ struct aspeed_i2c_config {
>         u32 (*get_clk_reg_val)(struct device *dev, u32 divisor);
>         int (*enable_sram)(void);
>         int (*set_buf_xfer_mode)(struct device *dev);
> +       int (*set_dma_xfer_mode)(struct device *dev);
>  };
>
>  struct aspeed_i2c_bus {
> @@ -190,6 +205,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;
> @@ -272,9 +293,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;
> @@ -288,7 +313,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));
> @@ -305,7 +341,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,
> @@ -321,7 +364,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));
> @@ -451,7 +510,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) |
> @@ -474,7 +541,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;
>
> @@ -527,18 +605,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);
> @@ -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.

> @@ -633,7 +725,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);
> @@ -650,7 +749,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) {
> @@ -660,14 +759,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;
> @@ -1287,22 +1396,63 @@ static int aspeed_i2c_25xx_set_buf_xfer_mode(struct device *dev)
>         return bus->buf_size ? 0 : -EINVAL;
>  }
>
> +static int aspeed_i2c_24xx_set_dma_xfer_mode(struct device *dev)
> +{
> +       /* AST24xx doesn't support DMA mode */
> +
> +       return -EBADR;
> +}
> +
> +static int aspeed_i2c_25xx_set_dma_xfer_mode(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct aspeed_i2c_bus *bus = platform_get_drvdata(pdev);
> +       int ret;
> +
> +       ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> +       if (!ret) {
> +               bus->buf_size = ASPEED_I2CD_DMA_LEN_MASK >>
> +                               ASPEED_I2CD_DMA_LEN_SHIFT;
> +               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) {
> +                       ret = -ENOMEM;
> +                       bus->buf_size = 0;
> +                       dev_dbg(&pdev->dev, "Cannot allocate DMA buffer\n");
> +                       dma_pool_destroy(bus->dma_pool);
> +               }
> +       }
> +
> +       return ret;
> +}
> +
>  static const struct aspeed_i2c_config ast24xx_config = {
>         .get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val,
>         .enable_sram = aspeed_i2c_24xx_enable_sram,
>         .set_buf_xfer_mode = aspeed_i2c_24xx_set_buf_xfer_mode,
> +       .set_dma_xfer_mode = aspeed_i2c_24xx_set_dma_xfer_mode,
>  };
>
>  static const struct aspeed_i2c_config ast25xx_config = {
>         .get_clk_reg_val = aspeed_i2c_25xx_get_clk_reg_val,
>         .enable_sram = aspeed_i2c_25xx_enable_sram,
>         .set_buf_xfer_mode = aspeed_i2c_25xx_set_buf_xfer_mode,
> +       .set_dma_xfer_mode = aspeed_i2c_25xx_set_dma_xfer_mode,
>  };
>
>  static const struct aspeed_i2c_config ast26xx_config = {
>         .get_clk_reg_val = aspeed_i2c_25xx_get_clk_reg_val,
>         .enable_sram = aspeed_i2c_24xx_enable_sram,
>         .set_buf_xfer_mode = aspeed_i2c_25xx_set_buf_xfer_mode,
> +       .set_dma_xfer_mode = aspeed_i2c_25xx_set_dma_xfer_mode,
>  };
>
>  static const struct of_device_id aspeed_i2c_bus_of_table[] = {
> @@ -1324,8 +1474,12 @@ static void aspeed_i2c_set_xfer_mode(struct aspeed_i2c_bus *bus)
>                 return;
>
>         ret = bus->config->enable_sram();
> -       if (!ret && !strncasecmp(mode, "buf", 3))
> -               ret = bus->config->set_buf_xfer_mode(bus->dev);
> +       if (!ret) {
> +               if (!strncasecmp(mode, "buf", 3))
> +                       ret = bus->config->set_buf_xfer_mode(bus->dev);
> +               else if (!strncasecmp(mode, "dma", 3))
> +                       ret = bus->config->set_dma_xfer_mode(bus->dev);
> +       }
>
>         if (ret)
>                 dev_dbg(&pdev->dev, "Use default (byte) xfer mode\n");
> @@ -1400,22 +1554,31 @@ 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;
>
>         dev_info(bus->dev, "i2c bus %d registered (%s mode), irq %d\n",
> -                bus->adap.nr, bus->buf_base ? "buf" : "byte", irq);
> +                bus->adap.nr, bus->dma_buf ? "dma" :
> +                                             bus->buf_base ? "buf" : "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)
> @@ -1433,6 +1596,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 v4 4/4] i2c: aspeed: add DMA mode transfer support

[Jae Hyun Yoo]

Hi Brendan,

On 4/13/2021 2:32 PM, Brendan Higgins wrote:
> On Wed, Feb 24, 2021 at 11:04 AM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
>>
>> This commit adds DMA mode transfer support.
>>
>> Only AST2500 and later versions support 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 v3:
>> - None
>>
>> Changes since v2:
>> - Refined SoC family dependent xfer mode configuration functions.
>>
>> Changes since v1:
>> - Updated commit message and comments.
>> - Refined using abstract functions.
>>
>>   drivers/i2c/busses/i2c-aspeed.c | 265 ++++++++++++++++++++++++++------
>>   1 file changed, 216 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index ffc52937df26..3e3bb014b027 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,
>> @@ -161,6 +175,7 @@ struct aspeed_i2c_config {
>>          u32 (*get_clk_reg_val)(struct device *dev, u32 divisor);
>>          int (*enable_sram)(void);
>>          int (*set_buf_xfer_mode)(struct device *dev);
>> +       int (*set_dma_xfer_mode)(struct device *dev);
>>   };
>>
>>   struct aspeed_i2c_bus {
>> @@ -190,6 +205,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;
>> @@ -272,9 +293,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;
>> @@ -288,7 +313,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));
>> @@ -305,7 +341,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,
>> @@ -321,7 +364,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));
>> @@ -451,7 +510,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) |
>> @@ -474,7 +541,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;
>>
>> @@ -527,18 +605,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);
>> @@ -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

>> @@ -633,7 +725,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);
>> @@ -650,7 +749,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) {
>> @@ -660,14 +759,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;
>> @@ -1287,22 +1396,63 @@ static int aspeed_i2c_25xx_set_buf_xfer_mode(struct device *dev)
>>          return bus->buf_size ? 0 : -EINVAL;
>>   }
>>
>> +static int aspeed_i2c_24xx_set_dma_xfer_mode(struct device *dev)
>> +{
>> +       /* AST24xx doesn't support DMA mode */
>> +
>> +       return -EBADR;
>> +}
>> +
>> +static int aspeed_i2c_25xx_set_dma_xfer_mode(struct device *dev)
>> +{
>> +       struct platform_device *pdev = to_platform_device(dev);
>> +       struct aspeed_i2c_bus *bus = platform_get_drvdata(pdev);
>> +       int ret;
>> +
>> +       ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
>> +       if (!ret) {
>> +               bus->buf_size = ASPEED_I2CD_DMA_LEN_MASK >>
>> +                               ASPEED_I2CD_DMA_LEN_SHIFT;
>> +               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) {
>> +                       ret = -ENOMEM;
>> +                       bus->buf_size = 0;
>> +                       dev_dbg(&pdev->dev, "Cannot allocate DMA buffer\n");
>> +                       dma_pool_destroy(bus->dma_pool);
>> +               }
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>>   static const struct aspeed_i2c_config ast24xx_config = {
>>          .get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val,
>>          .enable_sram = aspeed_i2c_24xx_enable_sram,
>>          .set_buf_xfer_mode = aspeed_i2c_24xx_set_buf_xfer_mode,
>> +       .set_dma_xfer_mode = aspeed_i2c_24xx_set_dma_xfer_mode,
>>   };
>>
>>   static const struct aspeed_i2c_config ast25xx_config = {
>>          .get_clk_reg_val = aspeed_i2c_25xx_get_clk_reg_val,
>>          .enable_sram = aspeed_i2c_25xx_enable_sram,
>>          .set_buf_xfer_mode = aspeed_i2c_25xx_set_buf_xfer_mode,
>> +       .set_dma_xfer_mode = aspeed_i2c_25xx_set_dma_xfer_mode,
>>   };
>>
>>   static const struct aspeed_i2c_config ast26xx_config = {
>>          .get_clk_reg_val = aspeed_i2c_25xx_get_clk_reg_val,
>>          .enable_sram = aspeed_i2c_24xx_enable_sram,
>>          .set_buf_xfer_mode = aspeed_i2c_25xx_set_buf_xfer_mode,
>> +       .set_dma_xfer_mode = aspeed_i2c_25xx_set_dma_xfer_mode,
>>   };
>>
>>   static const struct of_device_id aspeed_i2c_bus_of_table[] = {
>> @@ -1324,8 +1474,12 @@ static void aspeed_i2c_set_xfer_mode(struct aspeed_i2c_bus *bus)
>>                  return;
>>
>>          ret = bus->config->enable_sram();
>> -       if (!ret && !strncasecmp(mode, "buf", 3))
>> -               ret = bus->config->set_buf_xfer_mode(bus->dev);
>> +       if (!ret) {
>> +               if (!strncasecmp(mode, "buf", 3))
>> +                       ret = bus->config->set_buf_xfer_mode(bus->dev);
>> +               else if (!strncasecmp(mode, "dma", 3))
>> +                       ret = bus->config->set_dma_xfer_mode(bus->dev);
>> +       }
>>
>>          if (ret)
>>                  dev_dbg(&pdev->dev, "Use default (byte) xfer mode\n");
>> @@ -1400,22 +1554,31 @@ 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;
>>
>>          dev_info(bus->dev, "i2c bus %d registered (%s mode), irq %d\n",
>> -                bus->adap.nr, bus->buf_base ? "buf" : "byte", irq);
>> +                bus->adap.nr, bus->dma_buf ? "dma" :
>> +                                             bus->buf_base ? "buf" : "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)
>> @@ -1433,6 +1596,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 v4 4/4] i2c: aspeed: add DMA mode transfer support

[Jae Hyun Yoo]

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

[....]

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

[Zev Weiss]

On Wed, Feb 24, 2021 at 11:17:16AM PST, Jae Hyun Yoo wrote:
>This patch series adds buffer mode and DMA mode transfer support for the
>Aspeed I2C driver. With this change, buffer mode and DMA mode can be
>selectively used depend on platform configuration.
>

Any updates on these patches?  They provide a welcome performance
improvement for some stuff I've been doing -- for the v4 series:

Tested-by: Zev Weiss <zweiss@equinix.com>

Re: [PATCH v4 1/4] dt-bindings: i2c: aspeed: add transfer mode support

[Jae Hyun Yoo]

On 4/13/2021 12:50 PM, Brendan Higgins wrote:
> On Thu, Apr 8, 2021 at 10:50 AM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
>>
>> Ping.
>>
>> On 3/10/2021 7:55 AM, Jae Hyun Yoo wrote:
>>> On 3/9/2021 6:15 PM, Rob Herring wrote:
>>>> On Tue, Mar 9, 2021 at 10:02 AM Jae Hyun Yoo
>>>> <jae.hyun.yoo@linux.intel.com> wrote:
>>>>>
>>>>> Hi Rob,
>>>>>
>>>>> On 3/6/2021 12:30 PM, Rob Herring wrote:
>>>>>> On Wed, Feb 24, 2021 at 11:17:17AM -0800, Jae Hyun Yoo wrote:
>>>>>>> Append bindings to support transfer mode.
>>>>>>>
>>>>>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>>>>>>> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>>>>>>> ---
>>>>>>> Changes since v3:
>>>>>>> - None
>>>>>>>
>>>>>>> Changes since v2:
>>>>>>> - Moved SRAM resources back to default dtsi and added mode selection
>>>>>>>      property.
>>>>>>>
>>>>>>> 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    | 37
>>>>>>> +++++++++++++++----
>>>>>>>     1 file changed, 30 insertions(+), 7 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>>>>>>> b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>>>>>>> index b47f6ccb196a..242343177324 100644
>>>>>>> --- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>>>>>>> @@ -17,6 +17,20 @@ 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,i2c-xfer-mode      : should be "byte", "buf" or "dma" to
>>>>>>> select transfer
>>>>>>> +                      mode defaults to "byte" mode when not
>>>>>>> specified.
>>>>>>> +
>>>>>>> +                      I2C DMA mode on 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
>>>>>>
>>>>>> How does one decide between byte or buf mode?
>>>>>
>>>>> If a given system makes just one byte r/w transactions most of the time
>>>>> then byte mode will be a right setting. Otherwise, buf mode is more
>>>>> efficient because it doesn't generate a bunch of interrupts on every
>>>>> byte handling.
>>>>
>>>> Then why doesn't the driver do byte transactions when it gets small
>>>> 1-4? byte transactions and buffer transactions when it gets larger
>>>> sized transactions.
>>>
>>> Good question and it could be an option of this implementation.
>>> Actually, each mode needs different register handling so we need to add
>>> additional conditional branches to make it dynamic mode change depends
>>> on the data size which can be a downside. Also, checked that small
>>> amount of data transfer efficiency in 'buf' transfer mode is almost same
>>> to 'byte' mode so there would be no big benefit from the dynamic mode
>>> change. Of course, we can remove the 'byte' transfer mode but we should
>>> also provide flexibility of configuration on what this hardware can
>>> support, IMO.
> 
> I would rather set the choice in device tree or Kconfig, which the
> former is what I think you did here.
> 
> As for doing byte mode for small transactions and buffer/DMA for large
> transactions, I would prefer sticking to a single mode based on what
> is selected at build/boot time. Seems less error prone to me. Then
> again, Rob probably has more experience in this area than I do, so
> maybe this kind of thing is pretty common and I just don't realize it.
> 
> In any case, as for getting rid of byte mode, I would support that,
> but not in this patch set. I would rather switch the default and get
> users on buffer/DMA mode before taking away a fallback option.
> 
> My 2 cents, but I think the OzLabs and other active OpenBMC people are
> probably a little more up to date on this.
> 
> Cheers

Hi Brendan, Thanks for sharing your thought.

Hi Rob,

I'm bringing it back to make progress again. Like Brendan said, I also
prefer sticking to a single mode which is selected through device tree
and it would be right way for enabling all features this hardware
provides. There would be no big benefit if we make dynamic mode change
for 'byte' and 'buf' modes based on transaction size because 'buf' mode
can effectively cover the small transaction size cases well without
making a performance degradation. Means that we can remove the 'byte'
mode but I don't want to hide/block a feature which this hardware
originally provides.

Please let us know your thought.

Thanks,
Jae

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

[Jae Hyun Yoo]

Hi Zev,

On 9/29/2021 7:44 PM, Zev Weiss wrote:
> On Wed, Feb 24, 2021 at 11:17:16AM PST, Jae Hyun Yoo wrote:
>> This patch series adds buffer mode and DMA mode transfer support for the
>> Aspeed I2C driver. With this change, buffer mode and DMA mode can be
>> selectively used depend on platform configuration.
>>
> 
> Any updates on these patches?  They provide a welcome performance
> improvement for some stuff I've been doing -- for the v4 series:
> 
> Tested-by: Zev Weiss <zweiss@equinix.com>

Thanks for sharing your test result.

I'm bringing this patch set back to discussion.

Thanks,
Jae