* [PATCH 0/5] i2c: aspeed: Add buffer and DMA modes support
@ 2019-10-07 23:13 Jae Hyun Yoo
2019-10-07 23:13 ` [PATCH 1/5] dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support Jae Hyun Yoo
` (4 more replies)
0 siblings, 5 replies; 22+ messages in thread
From: Jae Hyun Yoo @ 2019-10-07 23:13 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Rob Herring, Mark Rutland, Andrew Jeffery, Tao Ren
Cc: devicetree, Jae Hyun Yoo, linux-aspeed, openbmc, linux-i2c,
linux-arm-kernel
This patch series adds buffer mode and DMA mode transfer support for the
Aspeed I2C driver. With this change, default transfer mode will be set to
buffer mode for better performance, and DMA mode can be selectively used
depends on platform configuration.
* Buffer mode
AST2400:
It has 2 KBytes (256 Bytes x 8 pages) of I2C SRAM buffer pool from
0x1e78a800 to 0x1e78afff that can be used for all busses with
buffer pool manipulation. To simplify implementation for supporting
both AST2400 and AST2500, it assigns each 128 Bytes per bus without
using buffer pool manipulation so total 1792 Bytes of I2C SRAM
buffer will be used.
AST2500:
It has 16 Bytes of individual I2C SRAM buffer per each bus and its
range is from 0x1e78a200 to 0x1e78a2df, so it doesn't have 'buffer
page selection' bit field in the Function control register, and
neither 'base address pointer' bit field in the Pool buffer control
register it has. To simplify implementation for supporting both
AST2400 and AST2500, it writes zeros on those register bit fields
but it's okay because it does nothing in AST2500.
* DMA mode
Only AST2500 and later versions support DMA mode under some limitations:
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.
-Jae
Jae Hyun Yoo (5):
dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support
ARM: dts: aspeed: add I2C buffer mode support
i2c: aspeed: fix master pending state handling
i2c: aspeed: add buffer mode transfer support
i2c: aspeed: add DMA mode transfer support
.../devicetree/bindings/i2c/i2c-aspeed.txt | 67 ++-
arch/arm/boot/dts/aspeed-g4.dtsi | 47 +-
arch/arm/boot/dts/aspeed-g5.dtsi | 47 +-
drivers/i2c/busses/i2c-aspeed.c | 513 ++++++++++++++++--
4 files changed, 588 insertions(+), 86 deletions(-)
--
2.23.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/5] dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support
2019-10-07 23:13 [PATCH 0/5] i2c: aspeed: Add buffer and DMA modes support Jae Hyun Yoo
@ 2019-10-07 23:13 ` Jae Hyun Yoo
2019-10-08 18:12 ` Brendan Higgins
2019-10-07 23:13 ` [PATCH 2/5] ARM: dts: aspeed: add I2C buffer mode support Jae Hyun Yoo
` (3 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Jae Hyun Yoo @ 2019-10-07 23:13 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Rob Herring, Mark Rutland, Andrew Jeffery, Tao Ren
Cc: devicetree, Jae Hyun Yoo, linux-aspeed, openbmc, linux-i2c,
linux-arm-kernel
Append bindings to support buffer mode and DMA mode transfer.
Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
.../devicetree/bindings/i2c/i2c-aspeed.txt | 67 +++++++++++++++++--
1 file changed, 60 insertions(+), 7 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
index 8fbd8633a387..e40dcc108307 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
@@ -3,7 +3,10 @@ Device tree configuration for the I2C busses on the AST24XX and AST25XX SoCs.
Required Properties:
- #address-cells : should be 1
- #size-cells : should be 0
-- reg : address offset and range of bus
+- reg : Address offset and range of bus registers.
+ An additional SRAM buffer address offset and range is
+ optional in case of enabling I2C dedicated SRAM for
+ buffer mode transfer support.
- compatible : should be "aspeed,ast2400-i2c-bus"
or "aspeed,ast2500-i2c-bus"
- clocks : root clock of bus, should reference the APB
@@ -16,6 +19,18 @@ Optional Properties:
- bus-frequency : frequency of the bus clock in Hz defaults to 100 kHz when not
specified
- multi-master : states that there is another master active on this bus.
+- aspeed,dma-buf-size : size of DMA buffer (from 2 to 4095 in case of AST2500
+ or later versions).
+ Only AST2500 and later versions support DMA mode
+ under some limitations:
+ I2C is sharing the DMA H/W with UHCI host controller
+ and MCTP controller. Since those controllers operate
+ with DMA mode only, I2C has to use buffer mode or byte
+ mode instead if one of those controllers is enabled.
+ Also make sure that if SD/eMMC or Port80 snoop uses
+ DMA mode instead of PIO or FIFO respectively, I2C
+ can't use DMA mode. If both DMA and buffer modes are
+ enabled, DMA mode will be selected.
Example:
@@ -25,12 +40,21 @@ i2c {
#size-cells = <1>;
ranges = <0 0x1e78a000 0x1000>;
- i2c_ic: interrupt-controller@0 {
- #interrupt-cells = <1>;
- compatible = "aspeed,ast2400-i2c-ic";
+ i2c_gr: i2c-global-regs@0 {
+ compatible = "aspeed,ast2500-i2c-gr", "syscon";
reg = <0x0 0x40>;
- interrupts = <12>;
- interrupt-controller;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x0 0x40>;
+
+ i2c_ic: interrupt-controller@0 {
+ #interrupt-cells = <1>;
+ compatible = "aspeed,ast2500-i2c-ic";
+ reg = <0x0 0x4>;
+ interrupts = <12>;
+ interrupt-controller;
+ };
};
i2c0: i2c-bus@40 {
@@ -38,11 +62,40 @@ i2c {
#size-cells = <0>;
#interrupt-cells = <1>;
reg = <0x40 0x40>;
- compatible = "aspeed,ast2400-i2c-bus";
+ compatible = "aspeed,ast2500-i2c-bus";
clocks = <&syscon ASPEED_CLK_APB>;
resets = <&syscon ASPEED_RESET_I2C>;
bus-frequency = <100000>;
interrupts = <0>;
interrupt-parent = <&i2c_ic>;
};
+
+ /* buffer mode transfer enabled */
+ i2c1: i2c-bus@80 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ #interrupt-cells = <1>;
+ reg = <0x80 0x40>, <0x210 0x10>;
+ compatible = "aspeed,ast2500-i2c-bus";
+ clocks = <&syscon ASPEED_CLK_APB>;
+ resets = <&syscon ASPEED_RESET_I2C>;
+ bus-frequency = <100000>;
+ interrupts = <1>;
+ interrupt-parent = <&i2c_ic>;
+ };
+
+ /* DMA mode transfer enabled */
+ i2c2: i2c-bus@c0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ #interrupt-cells = <1>;
+ reg = <0xc0 0x40>;
+ aspeed,dma-buf-size = <4095>;
+ compatible = "aspeed,ast2500-i2c-bus";
+ clocks = <&syscon ASPEED_CLK_APB>;
+ resets = <&syscon ASPEED_RESET_I2C>;
+ bus-frequency = <100000>;
+ interrupts = <2>;
+ interrupt-parent = <&i2c_ic>;
+ };
};
--
2.23.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/5] ARM: dts: aspeed: add I2C buffer mode support
2019-10-07 23:13 [PATCH 0/5] i2c: aspeed: Add buffer and DMA modes support Jae Hyun Yoo
2019-10-07 23:13 ` [PATCH 1/5] dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support Jae Hyun Yoo
@ 2019-10-07 23:13 ` Jae Hyun Yoo
2019-10-09 13:32 ` Cédric Le Goater
2019-10-07 23:13 ` [PATCH 3/5] i2c: aspeed: fix master pending state handling Jae Hyun Yoo
` (2 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Jae Hyun Yoo @ 2019-10-07 23:13 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Rob Herring, Mark Rutland, Andrew Jeffery, Tao Ren
Cc: devicetree, Jae Hyun Yoo, linux-aspeed, openbmc, linux-i2c,
linux-arm-kernel
Byte mode currently this driver uses makes lots of interrupt call
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.
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.
This commit fixes all I2C bus nodes to support buffer mode
transfer.
Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
arch/arm/boot/dts/aspeed-g4.dtsi | 47 +++++++++++++++++++-------------
arch/arm/boot/dts/aspeed-g5.dtsi | 47 +++++++++++++++++++-------------
2 files changed, 56 insertions(+), 38 deletions(-)
diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index dffb595d30e4..f51b016aa769 100644
--- a/arch/arm/boot/dts/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed-g4.dtsi
@@ -420,12 +420,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 {
@@ -433,7 +442,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>;
@@ -449,7 +458,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>;
@@ -465,7 +474,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>;
@@ -482,7 +491,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>;
@@ -499,7 +508,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>;
@@ -516,7 +525,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>;
@@ -533,7 +542,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>;
@@ -550,7 +559,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>;
@@ -567,7 +576,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>;
@@ -584,7 +593,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>;
@@ -601,7 +610,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>;
@@ -618,7 +627,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>;
@@ -635,7 +644,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>;
@@ -652,7 +661,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 e8feb8b66a2f..cbc31ce3fab2 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -513,12 +513,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 {
@@ -526,7 +535,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>;
@@ -542,7 +551,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>;
@@ -558,7 +567,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>;
@@ -575,7 +584,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>;
@@ -592,7 +601,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>;
@@ -609,7 +618,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>;
@@ -626,7 +635,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>;
@@ -643,7 +652,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>;
@@ -660,7 +669,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>;
@@ -677,7 +686,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>;
@@ -694,7 +703,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>;
@@ -711,7 +720,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>;
@@ -728,7 +737,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>;
@@ -745,7 +754,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>;
--
2.23.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/5] i2c: aspeed: fix master pending state handling
2019-10-07 23:13 [PATCH 0/5] i2c: aspeed: Add buffer and DMA modes support Jae Hyun Yoo
2019-10-07 23:13 ` [PATCH 1/5] dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support Jae Hyun Yoo
2019-10-07 23:13 ` [PATCH 2/5] ARM: dts: aspeed: add I2C buffer mode support Jae Hyun Yoo
@ 2019-10-07 23:13 ` Jae Hyun Yoo
2019-10-08 20:31 ` Brendan Higgins
2019-10-08 22:00 ` Tao Ren
2019-10-07 23:13 ` [PATCH 4/5] i2c: aspeed: add buffer mode transfer support Jae Hyun Yoo
2019-10-07 23:13 ` [PATCH 5/5] i2c: aspeed: add DMA " Jae Hyun Yoo
4 siblings, 2 replies; 22+ messages in thread
From: Jae Hyun Yoo @ 2019-10-07 23:13 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Rob Herring, Mark Rutland, Andrew Jeffery, Tao Ren
Cc: devicetree, Jae Hyun Yoo, linux-aspeed, openbmc, linux-i2c,
linux-arm-kernel
In case of master pending state, it should not trigger the master
command because this H/W is sharing the same data buffer for slave
and master operations, so this commit fixes the issue with making
the master command triggering happen when the state goes to active
state.
Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
drivers/i2c/busses/i2c-aspeed.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index fa66951b05d0..40f6cf98d32e 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -336,18 +336,19 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
u8 slave_addr = i2c_8bit_addr_from_msg(msg);
- bus->master_state = ASPEED_I2C_MASTER_START;
-
#if IS_ENABLED(CONFIG_I2C_SLAVE)
/*
* If it's requested in the middle of a slave session, set the master
* state to 'pending' then H/W will continue handling this master
* command when the bus comes back to the idle state.
*/
- if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
+ if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) {
bus->master_state = ASPEED_I2C_MASTER_PENDING;
+ return;
+ }
#endif /* CONFIG_I2C_SLAVE */
+ bus->master_state = ASPEED_I2C_MASTER_START;
bus->buf_index = 0;
if (msg->flags & I2C_M_RD) {
@@ -432,7 +433,7 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
goto out_no_complete;
- bus->master_state = ASPEED_I2C_MASTER_START;
+ aspeed_i2c_do_start(bus);
}
#endif /* CONFIG_I2C_SLAVE */
--
2.23.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 4/5] i2c: aspeed: add buffer mode transfer support
2019-10-07 23:13 [PATCH 0/5] i2c: aspeed: Add buffer and DMA modes support Jae Hyun Yoo
` (2 preceding siblings ...)
2019-10-07 23:13 ` [PATCH 3/5] i2c: aspeed: fix master pending state handling Jae Hyun Yoo
@ 2019-10-07 23:13 ` Jae Hyun Yoo
2019-10-08 20:12 ` Brendan Higgins
2019-10-07 23:13 ` [PATCH 5/5] i2c: aspeed: add DMA " Jae Hyun Yoo
4 siblings, 1 reply; 22+ messages in thread
From: Jae Hyun Yoo @ 2019-10-07 23:13 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Rob Herring, Mark Rutland, Andrew Jeffery, Tao Ren
Cc: devicetree, Jae Hyun Yoo, linux-aspeed, openbmc, linux-i2c,
linux-arm-kernel
Byte mode currently this driver uses makes lots of interrupt call
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>
---
drivers/i2c/busses/i2c-aspeed.c | 297 ++++++++++++++++++++++++++++----
1 file changed, 263 insertions(+), 34 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 40f6cf98d32e..37d1a7fa2f87 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)
@@ -102,6 +110,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)
@@ -112,6 +122,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,
@@ -157,6 +174,11 @@ struct aspeed_i2c_bus {
int master_xfer_result;
/* Multi-master */
bool multi_master;
+ /* Buffer mode */
+ void __iomem *buf_base;
+ size_t buf_size;
+ u8 buf_offset;
+ u8 buf_page;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
struct i2c_client *slave;
enum aspeed_i2c_slave_state slave_state;
@@ -238,6 +260,7 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
{
u32 command, irq_handled = 0;
struct i2c_client *slave = bus->slave;
+ int i, len;
u8 value;
if (!slave)
@@ -260,7 +283,12 @@ 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;
+ 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;
/* Handle address frame. */
if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
if (value & 0x1)
@@ -275,6 +303,20 @@ 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) {
+ 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(slave,
+ I2C_SLAVE_WRITE_RECEIVED,
+ &value);
+ }
+ }
+ }
irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
@@ -307,9 +349,36 @@ 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);
+ 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);
+ }
break;
case ASPEED_I2C_SLAVE_WRITE_RECEIVED:
i2c_slave_event(slave, I2C_SLAVE_WRITE_RECEIVED, &value);
+ 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(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);
+ }
break;
case ASPEED_I2C_SLAVE_STOP:
i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
@@ -335,6 +404,8 @@ 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);
+ u8 wbuf[4];
+ int len;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
/*
@@ -353,12 +424,66 @@ 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 (bus->buf_base && !(msg->flags & I2C_M_RECV_LEN)) {
+ command |= ASPEED_I2CD_RX_BUFF_ENABLE;
+
+ if (msg->len > bus->buf_size) {
+ len = bus->buf_size;
+ } else {
+ len = msg->len;
+ command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
+ }
+
+ 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);
+ } else {
+ /* 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;
+ }
+ } else {
+ if (bus->buf_base) {
+ int i;
+
+ command |= ASPEED_I2CD_TX_BUFF_ENABLE;
+
+ if (msg->len + 1 > bus->buf_size)
+ len = bus->buf_size;
+ else
+ len = msg->len + 1;
+
+ /*
+ * Yeah, it looks clumsy but byte writings on a remapped
+ * I2C SRAM cause corruptions 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));
+
+ bus->buf_index = len - 1;
+
+ 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(slave_addr, bus->base + ASPEED_I2C_BYTE_BUF_REG);
+ if (!(command & ASPEED_I2CD_TX_BUFF_ENABLE))
+ writel(slave_addr, bus->base + ASPEED_I2C_BYTE_BUF_REG);
writel(command, bus->base + ASPEED_I2C_CMD_REG);
}
@@ -398,7 +523,7 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
u32 irq_handled = 0, command = 0;
struct i2c_msg *msg;
u8 recv_byte;
- int ret;
+ int ret, len;
if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
@@ -511,11 +636,43 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
/* fall through */
case ASPEED_I2C_MASTER_TX_FIRST:
if (bus->buf_index < msg->len) {
+ command = ASPEED_I2CD_M_TX_CMD;
+
+ if (bus->buf_base) {
+ 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));
+
+ bus->buf_index += len;
+
+ 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);
+ } else {
+ writel(msg->buf[bus->buf_index++],
+ bus->base + ASPEED_I2C_BYTE_BUF_REG);
+ }
+ 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);
}
@@ -532,25 +689,56 @@ 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) {
- if (unlikely(recv_byte > I2C_SMBUS_BLOCK_MAX)) {
- bus->cmd_err = -EPROTO;
- aspeed_i2c_do_stop(bus);
- goto out_no_complete;
+ if (bus->buf_base && !(msg->flags & I2C_M_RECV_LEN)) {
+ 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;
+
+ if (msg->flags & I2C_M_RECV_LEN) {
+ 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->flags &= ~I2C_M_RECV_LEN;
}
- msg->len = recv_byte +
- ((msg->flags & I2C_CLIENT_PEC) ? 2 : 1);
- msg->flags &= ~I2C_M_RECV_LEN;
}
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;
+ bus->master_state = ASPEED_I2C_MASTER_RX;
+ if (bus->buf_base) {
+ command |= ASPEED_I2CD_RX_BUFF_ENABLE;
+
+ 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;
+ }
+
+ 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;
+ }
writel(command, bus->base + ASPEED_I2C_CMD_REG);
} else {
aspeed_i2c_next_msg_or_stop(bus);
@@ -890,6 +1078,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
@@ -947,16 +1138,15 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
{
const struct of_device_id *match;
struct aspeed_i2c_bus *bus;
+ bool sram_enabled = true;
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);
@@ -990,6 +1180,45 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
match->data;
+ /*
+ * Enable I2C SRAM in case of AST2500.
+ * SRAM is enabled by default in AST2400 and AST2600.
+ */
+ if (of_device_is_compatible(pdev->dev.of_node,
+ "aspeed,ast2500-i2c-bus")) {
+ struct regmap *gr_regmap = syscon_regmap_lookup_by_compatible("aspeed,ast2500-i2c-gr");
+
+ if (IS_ERR(gr_regmap))
+ ret = PTR_ERR(gr_regmap);
+ else
+ ret = regmap_update_bits(gr_regmap,
+ ASPEED_I2CG_GLOBAL_CTRL_REG,
+ ASPEED_I2CG_SRAM_BUFFER_EN,
+ ASPEED_I2CG_SRAM_BUFFER_EN);
+
+ if (ret)
+ sram_enabled = false;
+ }
+
+ if (sram_enabled) {
+ struct resource *res = platform_get_resource(pdev,
+ IORESOURCE_MEM, 1);
+
+ if (res && resource_size(res) >= 2)
+ bus->buf_base = devm_ioremap_resource(&pdev->dev, res);
+
+ if (!IS_ERR_OR_NULL(bus->buf_base)) {
+ bus->buf_size = resource_size(res);
+ if (of_device_is_compatible(pdev->dev.of_node,
+ "aspeed,ast2400-i2c-bus")) {
+ bus->buf_page = ((res->start >> 8) &
+ GENMASK(3, 0)) - 8;
+ bus->buf_offset = (res->start >> 2) &
+ ASPEED_I2CD_BUF_OFFSET_MASK;
+ }
+ }
+ }
+
/* Initialize the I2C adapter */
spin_lock_init(&bus->lock);
init_completion(&bus->cmd_complete);
@@ -1026,8 +1255,8 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
platform_set_drvdata(pdev, bus);
- dev_info(bus->dev, "i2c bus %d registered, irq %d\n",
- bus->adap.nr, irq);
+ dev_info(bus->dev, "i2c bus %d registered (%s mode), irq %d\n",
+ bus->adap.nr, bus->buf_base ? "buffer" : "byte", irq);
return 0;
}
--
2.23.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 5/5] i2c: aspeed: add DMA mode transfer support
2019-10-07 23:13 [PATCH 0/5] i2c: aspeed: Add buffer and DMA modes support Jae Hyun Yoo
` (3 preceding siblings ...)
2019-10-07 23:13 ` [PATCH 4/5] i2c: aspeed: add buffer mode transfer support Jae Hyun Yoo
@ 2019-10-07 23:13 ` Jae Hyun Yoo
4 siblings, 0 replies; 22+ messages in thread
From: Jae Hyun Yoo @ 2019-10-07 23:13 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Rob Herring, Mark Rutland, Andrew Jeffery, Tao Ren
Cc: devicetree, Jae Hyun Yoo, linux-aspeed, openbmc, linux-i2c,
linux-arm-kernel
This commit adds DMA mode transfer support for better performance.
Only AST2500 and AST2600 support DMA mode under some limitations:
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.
Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
drivers/i2c/busses/i2c-aspeed.c | 231 +++++++++++++++++++++++++++++---
1 file changed, 216 insertions(+), 15 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 37d1a7fa2f87..d46e446ea48c 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 */
@@ -110,6 +114,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)
@@ -129,6 +135,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,
@@ -179,6 +193,12 @@ struct aspeed_i2c_bus {
size_t buf_size;
u8 buf_offset;
u8 buf_page;
+ /* DMA mode */
+ struct dma_pool *dma_pool;
+ dma_addr_t dma_handle;
+ u8 *dma_buf;
+ size_t dma_buf_size;
+ size_t dma_len;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
struct i2c_client *slave;
enum aspeed_i2c_slave_state slave_state;
@@ -283,9 +303,13 @@ 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) {
- 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;
@@ -305,7 +329,18 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
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->dma_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(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));
@@ -349,7 +384,15 @@ 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);
- 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->dma_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,
@@ -361,7 +404,25 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
break;
case ASPEED_I2C_SLAVE_WRITE_RECEIVED:
i2c_slave_event(slave, I2C_SLAVE_WRITE_RECEIVED, &value);
- if (bus->buf_base) {
+ if (bus->dma_buf) {
+ len = bus->dma_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(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->dma_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));
@@ -425,7 +486,23 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
if (msg->flags & I2C_M_RD) {
command |= ASPEED_I2CD_M_RX_CMD;
- if (bus->buf_base && !(msg->flags & I2C_M_RECV_LEN)) {
+ if (bus->dma_buf && !(msg->flags & I2C_M_RECV_LEN)) {
+ command |= ASPEED_I2CD_RX_DMA_ENABLE;
+
+ if (msg->len > bus->dma_buf_size) {
+ len = bus->dma_buf_size;
+ } else {
+ len = msg->len;
+ command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
+ }
+
+ 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 if (bus->buf_base && !(msg->flags & I2C_M_RECV_LEN)) {
command |= ASPEED_I2CD_RX_BUFF_ENABLE;
if (msg->len > bus->buf_size) {
@@ -446,7 +523,26 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
}
} else {
- if (bus->buf_base) {
+ if (bus->dma_buf) {
+ command |= ASPEED_I2CD_TX_DMA_ENABLE;
+
+ if (msg->len + 1 > bus->dma_buf_size)
+ len = bus->dma_buf_size;
+ else
+ len = msg->len + 1;
+
+ bus->dma_buf[0] = slave_addr;
+ memcpy(bus->dma_buf + 1, msg->buf, len);
+
+ bus->buf_index = len - 1;
+
+ 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 if (bus->buf_base) {
int i;
command |= ASPEED_I2CD_TX_BUFF_ENABLE;
@@ -482,7 +578,8 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
}
}
- if (!(command & ASPEED_I2CD_TX_BUFF_ENABLE))
+ if (!(command & (ASPEED_I2CD_TX_BUFF_ENABLE |
+ ASPEED_I2CD_TX_DMA_ENABLE)))
writel(slave_addr, bus->base + ASPEED_I2C_BYTE_BUF_REG);
writel(command, bus->base + ASPEED_I2C_CMD_REG);
}
@@ -638,7 +735,28 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
if (bus->buf_index < msg->len) {
command = ASPEED_I2CD_M_TX_CMD;
- if (bus->buf_base) {
+ if (bus->dma_buf) {
+ command |= ASPEED_I2CD_TX_DMA_ENABLE;
+
+ if (msg->len - bus->buf_index >
+ bus->dma_buf_size)
+ len = bus->dma_buf_size;
+ else
+ len = msg->len - bus->buf_index;
+
+ memcpy(bus->dma_buf, msg->buf + bus->buf_index,
+ len);
+
+ 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 if (bus->buf_base) {
u8 wbuf[4];
int i;
@@ -689,7 +807,15 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
}
irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
- if (bus->buf_base && !(msg->flags & I2C_M_RECV_LEN)) {
+ if (bus->dma_buf && !(msg->flags & I2C_M_RECV_LEN)) {
+ 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 && !(msg->flags & I2C_M_RECV_LEN)) {
len = FIELD_GET(ASPEED_I2CD_BUF_RX_COUNT_MASK,
readl(bus->base +
ASPEED_I2C_BUF_CTRL_REG));
@@ -717,7 +843,25 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
if (bus->buf_index < msg->len) {
command = ASPEED_I2CD_M_RX_CMD;
bus->master_state = ASPEED_I2C_MASTER_RX;
- if (bus->buf_base) {
+ if (bus->dma_buf) {
+ command |= ASPEED_I2CD_RX_DMA_ENABLE;
+
+ if (msg->len - bus->buf_index >
+ bus->dma_buf_size) {
+ len = bus->dma_buf_size;
+ } else {
+ len = msg->len - bus->buf_index;
+ command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
+ }
+
+ 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 if (bus->buf_base) {
command |= ASPEED_I2CD_RX_BUFF_ENABLE;
if (msg->len - bus->buf_index >
@@ -1200,7 +1344,51 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
sram_enabled = false;
}
- if (sram_enabled) {
+ /*
+ * Only AST2500 and AST2600 support DMA mode under some limitations:
+ * I2C is sharing the DMA H/W with UHCI host controller and MCTP
+ * controller. Since those controllers operate with DMA mode only, I2C
+ * has to use buffer mode or byte mode instead if one of those
+ * controllers is enabled. Also make sure that if SD/eMMC or Port80
+ * snoop uses DMA mode instead of PIO or FIFO respectively, I2C can't
+ * use DMA mode.
+ */
+ if (sram_enabled && !IS_ENABLED(CONFIG_USB_UHCI_ASPEED) &&
+ !of_device_is_compatible(pdev->dev.of_node,
+ "aspeed,ast2400-i2c-bus")) {
+ u32 dma_len_max = ASPEED_I2CD_DMA_LEN_MASK >>
+ ASPEED_I2CD_DMA_LEN_SHIFT;
+
+ ret = device_property_read_u32(&pdev->dev,
+ "aspeed,dma-buf-size",
+ &bus->dma_buf_size);
+ if (!ret && bus->dma_buf_size > dma_len_max)
+ bus->dma_buf_size = dma_len_max;
+ }
+
+ if (bus->dma_buf_size) {
+ if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(32))) {
+ dev_warn(&pdev->dev, "No suitable DMA available\n");
+ } else {
+ bus->dma_pool = dma_pool_create("i2c-aspeed",
+ &pdev->dev,
+ bus->dma_buf_size,
+ ASPEED_I2CD_DMA_ALIGN,
+ 0);
+ if (bus->dma_pool)
+ bus->dma_buf = dma_pool_alloc(bus->dma_pool,
+ GFP_KERNEL,
+ &bus->dma_handle);
+
+ if (!bus->dma_buf) {
+ dev_warn(&pdev->dev,
+ "Cannot allocate DMA buffer\n");
+ dma_pool_destroy(bus->dma_pool);
+ }
+ }
+ }
+
+ if (!bus->dma_buf && sram_enabled) {
struct resource *res = platform_get_resource(pdev,
IORESOURCE_MEM, 1);
@@ -1241,24 +1429,33 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
*/
ret = aspeed_i2c_init(bus, pdev);
if (ret < 0)
- return ret;
+ goto out_free_dma_buf;
irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
ret = devm_request_irq(&pdev->dev, irq, aspeed_i2c_bus_irq,
0, dev_name(&pdev->dev), bus);
if (ret < 0)
- return ret;
+ goto out_free_dma_buf;
ret = i2c_add_adapter(&bus->adap);
if (ret < 0)
- return ret;
+ goto out_free_dma_buf;
platform_set_drvdata(pdev, bus);
dev_info(bus->dev, "i2c bus %d registered (%s mode), irq %d\n",
- bus->adap.nr, bus->buf_base ? "buffer" : "byte", irq);
+ bus->adap.nr, bus->dma_buf ? "dma" :
+ bus->buf_base ? "buffer" : "byte",
+ irq);
return 0;
+
+out_free_dma_buf:
+ if (bus->dma_buf)
+ dma_pool_free(bus->dma_pool, bus->dma_buf, bus->dma_handle);
+ dma_pool_destroy(bus->dma_pool);
+
+ return ret;
}
static int aspeed_i2c_remove_bus(struct platform_device *pdev)
@@ -1276,6 +1473,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.23.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/5] dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support
2019-10-07 23:13 ` [PATCH 1/5] dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support Jae Hyun Yoo
@ 2019-10-08 18:12 ` Brendan Higgins
2019-10-08 18:47 ` Jae Hyun Yoo
0 siblings, 1 reply; 22+ messages in thread
From: Brendan Higgins @ 2019-10-08 18:12 UTC (permalink / raw)
To: Jae Hyun Yoo
Cc: Mark Rutland, devicetree, linux-aspeed, Andrew Jeffery,
Benjamin Herrenschmidt, openbmc, Rob Herring, Joel Stanley,
Tao Ren, linux-arm-kernel, linux-i2c
On Mon, Oct 07, 2019 at 04:13:09PM -0700, Jae Hyun Yoo wrote:
> Append bindings to support buffer mode and DMA mode transfer.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
> .../devicetree/bindings/i2c/i2c-aspeed.txt | 67 +++++++++++++++++--
> 1 file changed, 60 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> index 8fbd8633a387..e40dcc108307 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> @@ -3,7 +3,10 @@ Device tree configuration for the I2C busses on the AST24XX and AST25XX SoCs.
> Required Properties:
> - #address-cells : should be 1
> - #size-cells : should be 0
> -- reg : address offset and range of bus
> +- reg : Address offset and range of bus registers.
> + An additional SRAM buffer address offset and range is
> + optional in case of enabling I2C dedicated SRAM for
> + buffer mode transfer support.
Sorry, I am having trouble parsing this. This seems like the SRAM buffer
is global to all busses. Can you clarify? I expect I will probably have
some more questions elsewhere.
> - compatible : should be "aspeed,ast2400-i2c-bus"
> or "aspeed,ast2500-i2c-bus"
> - clocks : root clock of bus, should reference the APB
> @@ -16,6 +19,18 @@ Optional Properties:
> - bus-frequency : frequency of the bus clock in Hz defaults to 100 kHz when not
> specified
> - multi-master : states that there is another master active on this bus.
> +- aspeed,dma-buf-size : size of DMA buffer (from 2 to 4095 in case of AST2500
> + or later versions).
> + Only AST2500 and later versions support DMA mode
> + under some limitations:
> + I2C is sharing the DMA H/W with UHCI host controller
> + and MCTP controller. Since those controllers operate
> + with DMA mode only, I2C has to use buffer mode or byte
> + mode instead if one of those controllers is enabled.
> + Also make sure that if SD/eMMC or Port80 snoop uses
> + DMA mode instead of PIO or FIFO respectively, I2C
> + can't use DMA mode. If both DMA and buffer modes are
> + enabled, DMA mode will be selected.
nit: I think it makes sense to break down the exceptions into a
bulleted list.
Cheers
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/5] dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support
2019-10-08 18:12 ` Brendan Higgins
@ 2019-10-08 18:47 ` Jae Hyun Yoo
0 siblings, 0 replies; 22+ messages in thread
From: Jae Hyun Yoo @ 2019-10-08 18:47 UTC (permalink / raw)
To: Brendan Higgins
Cc: Mark Rutland, devicetree, linux-aspeed, Andrew Jeffery,
Benjamin Herrenschmidt, openbmc, Rob Herring, Joel Stanley,
Tao Ren, linux-arm-kernel, linux-i2c
Hi Brendan,
On 10/8/2019 11:12 AM, Brendan Higgins wrote:
> On Mon, Oct 07, 2019 at 04:13:09PM -0700, Jae Hyun Yoo wrote:
>> Append bindings to support buffer mode and DMA mode transfer.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>> .../devicetree/bindings/i2c/i2c-aspeed.txt | 67 +++++++++++++++++--
>> 1 file changed, 60 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>> index 8fbd8633a387..e40dcc108307 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>> @@ -3,7 +3,10 @@ Device tree configuration for the I2C busses on the AST24XX and AST25XX SoCs.
>> Required Properties:
>> - #address-cells : should be 1
>> - #size-cells : should be 0
>> -- reg : address offset and range of bus
>> +- reg : Address offset and range of bus registers.
>> + An additional SRAM buffer address offset and range is
>> + optional in case of enabling I2C dedicated SRAM for
>> + buffer mode transfer support.
>
> Sorry, I am having trouble parsing this. This seems like the SRAM buffer
> is global to all busses. Can you clarify? I expect I will probably have
> some more questions elsewhere.
Each SoC has a different SRAM structure. In case of AST2400, it has a
SRAM buffer pool which can be shared by all busses. In the other hand,
AST2500/2600 have dedicated SRAM spaces for each bus.
This is what I explained in the cover letter:
* 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.
>> - compatible : should be "aspeed,ast2400-i2c-bus"
>> or "aspeed,ast2500-i2c-bus"
>> - clocks : root clock of bus, should reference the APB
>> @@ -16,6 +19,18 @@ Optional Properties:
>> - bus-frequency : frequency of the bus clock in Hz defaults to 100 kHz when not
>> specified
>> - multi-master : states that there is another master active on this bus.
>> +- aspeed,dma-buf-size : size of DMA buffer (from 2 to 4095 in case of AST2500
>> + or later versions).
>> + Only AST2500 and later versions support DMA mode
>> + under some limitations:
>> + I2C is sharing the DMA H/W with UHCI host controller
>> + and MCTP controller. Since those controllers operate
>> + with DMA mode only, I2C has to use buffer mode or byte
>> + mode instead if one of those controllers is enabled.
>> + Also make sure that if SD/eMMC or Port80 snoop uses
>> + DMA mode instead of PIO or FIFO respectively, I2C
>> + can't use DMA mode. If both DMA and buffer modes are
>> + enabled, DMA mode will be selected.
>
> nit: I think it makes sense to break down the exceptions into a
> bulleted list.
Okay. Will update it using bulleted list.
Thanks,
Jae
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] i2c: aspeed: add buffer mode transfer support
2019-10-07 23:13 ` [PATCH 4/5] i2c: aspeed: add buffer mode transfer support Jae Hyun Yoo
@ 2019-10-08 20:12 ` Brendan Higgins
2019-10-08 21:10 ` Jae Hyun Yoo
0 siblings, 1 reply; 22+ messages in thread
From: Brendan Higgins @ 2019-10-08 20:12 UTC (permalink / raw)
To: Jae Hyun Yoo
Cc: Mark Rutland, devicetree, linux-aspeed, Andrew Jeffery,
Benjamin Herrenschmidt, openbmc, Rob Herring, Joel Stanley,
Tao Ren, linux-arm-kernel, linux-i2c
On Mon, Oct 07, 2019 at 04:13:12PM -0700, Jae Hyun Yoo wrote:
> Byte mode currently this driver uses makes lots of interrupt call
nit: Drop "Byte mode".
> 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.
nit: Please use imperative mood.
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> Tested-by: Tao Ren <taoren@fb.com>
> ---
> drivers/i2c/busses/i2c-aspeed.c | 297 ++++++++++++++++++++++++++++----
> 1 file changed, 263 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 40f6cf98d32e..37d1a7fa2f87 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)
> @@ -102,6 +110,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)
> @@ -112,6 +122,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,
> @@ -157,6 +174,11 @@ struct aspeed_i2c_bus {
> int master_xfer_result;
> /* Multi-master */
> bool multi_master;
> + /* Buffer mode */
> + void __iomem *buf_base;
> + size_t buf_size;
> + u8 buf_offset;
> + u8 buf_page;
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> struct i2c_client *slave;
> enum aspeed_i2c_slave_state slave_state;
> @@ -238,6 +260,7 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
> {
> u32 command, irq_handled = 0;
> struct i2c_client *slave = bus->slave;
> + int i, len;
> u8 value;
>
> if (!slave)
> @@ -260,7 +283,12 @@ 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;
> + if (bus->buf_base &&
> + bus->slave_state == ASPEED_I2C_SLAVE_WRITE_RECEIVED &&
> + !(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))
I think checking for the buf_base all over the place makes this really
complicated and hard to read.
It might be better to just split this out and have separate handlers
based on what mode the driver is running in.
> + value = readb(bus->buf_base);
> + else
> + value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
> /* Handle address frame. */
> if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
> if (value & 0x1)
> @@ -275,6 +303,20 @@ 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) {
> + 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));
It looks like you have a lot of improvements in here unrelated to adding
support for buffer mode.
I really appreciate the improvements, but it makes it harder to
understand what buffer features you are adding vs. what
improvments/modernizations you are making.
Can you split this commit up?
> + for (i = 0; i < len; i++) {
> + value = readb(bus->buf_base + i);
> + i2c_slave_event(slave,
> + I2C_SLAVE_WRITE_RECEIVED,
> + &value);
> + }
> + }
> + }
> irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> }
> @@ -307,9 +349,36 @@ 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);
> + 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);
> + }
> break;
> case ASPEED_I2C_SLAVE_WRITE_RECEIVED:
> i2c_slave_event(slave, I2C_SLAVE_WRITE_RECEIVED, &value);
> + 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(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);
> + }
> break;
> case ASPEED_I2C_SLAVE_STOP:
> i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
> @@ -335,6 +404,8 @@ 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);
> + u8 wbuf[4];
> + int len;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> /*
> @@ -353,12 +424,66 @@ 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 (bus->buf_base && !(msg->flags & I2C_M_RECV_LEN)) {
> + command |= ASPEED_I2CD_RX_BUFF_ENABLE;
> +
> + if (msg->len > bus->buf_size) {
> + len = bus->buf_size;
> + } else {
> + len = msg->len;
> + command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
> + }
> +
> + 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);
> + } else {
> + /* 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;
> + }
> + } else {
> + if (bus->buf_base) {
> + int i;
> +
> + command |= ASPEED_I2CD_TX_BUFF_ENABLE;
> +
> + if (msg->len + 1 > bus->buf_size)
> + len = bus->buf_size;
> + else
> + len = msg->len + 1;
> +
> + /*
> + * Yeah, it looks clumsy but byte writings on a remapped
> + * I2C SRAM cause corruptions 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));
> +
> + bus->buf_index = len - 1;
> +
> + 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(slave_addr, bus->base + ASPEED_I2C_BYTE_BUF_REG);
> + if (!(command & ASPEED_I2CD_TX_BUFF_ENABLE))
> + writel(slave_addr, bus->base + ASPEED_I2C_BYTE_BUF_REG);
> writel(command, bus->base + ASPEED_I2C_CMD_REG);
> }
>
> @@ -398,7 +523,7 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
> u32 irq_handled = 0, command = 0;
> struct i2c_msg *msg;
> u8 recv_byte;
> - int ret;
> + int ret, len;
>
> if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> @@ -511,11 +636,43 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
> /* fall through */
> case ASPEED_I2C_MASTER_TX_FIRST:
> if (bus->buf_index < msg->len) {
> + command = ASPEED_I2CD_M_TX_CMD;
> +
> + if (bus->buf_base) {
> + 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));
> +
> + bus->buf_index += len;
> +
> + 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);
> + } else {
> + writel(msg->buf[bus->buf_index++],
> + bus->base + ASPEED_I2C_BYTE_BUF_REG);
> + }
> + 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);
> }
> @@ -532,25 +689,56 @@ 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) {
> - if (unlikely(recv_byte > I2C_SMBUS_BLOCK_MAX)) {
> - bus->cmd_err = -EPROTO;
> - aspeed_i2c_do_stop(bus);
> - goto out_no_complete;
> + if (bus->buf_base && !(msg->flags & I2C_M_RECV_LEN)) {
> + 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;
> +
> + if (msg->flags & I2C_M_RECV_LEN) {
> + 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->flags &= ~I2C_M_RECV_LEN;
> }
> - msg->len = recv_byte +
> - ((msg->flags & I2C_CLIENT_PEC) ? 2 : 1);
> - msg->flags &= ~I2C_M_RECV_LEN;
> }
>
> 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;
> + bus->master_state = ASPEED_I2C_MASTER_RX;
> + if (bus->buf_base) {
> + command |= ASPEED_I2CD_RX_BUFF_ENABLE;
> +
> + 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;
> + }
> +
> + 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;
> + }
> writel(command, bus->base + ASPEED_I2C_CMD_REG);
> } else {
> aspeed_i2c_next_msg_or_stop(bus);
> @@ -890,6 +1078,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
> @@ -947,16 +1138,15 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
> {
> const struct of_device_id *match;
> struct aspeed_i2c_bus *bus;
> + bool sram_enabled = true;
> 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);
>
> @@ -990,6 +1180,45 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
> bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
> match->data;
>
> + /*
> + * Enable I2C SRAM in case of AST2500.
> + * SRAM is enabled by default in AST2400 and AST2600.
> + */
This probe function is already pretty complicated as it is. Can we move
this to a helper function (especially since it only applies to the
25xx)?
> + if (of_device_is_compatible(pdev->dev.of_node,
> + "aspeed,ast2500-i2c-bus")) {
> + struct regmap *gr_regmap = syscon_regmap_lookup_by_compatible("aspeed,ast2500-i2c-gr");
So this memory is global, right? It is shared by all the busses?
If I am reading this right, then I think we need to protect so that only
one bus is accessing this memory at a time.
> + if (IS_ERR(gr_regmap))
> + ret = PTR_ERR(gr_regmap);
> + else
> + ret = regmap_update_bits(gr_regmap,
> + ASPEED_I2CG_GLOBAL_CTRL_REG,
> + ASPEED_I2CG_SRAM_BUFFER_EN,
> + ASPEED_I2CG_SRAM_BUFFER_EN);
> +
> + if (ret)
> + sram_enabled = false;
> + }
> +
> + if (sram_enabled) {
> + struct resource *res = platform_get_resource(pdev,
> + IORESOURCE_MEM, 1);
> +
> + if (res && resource_size(res) >= 2)
> + bus->buf_base = devm_ioremap_resource(&pdev->dev, res);
> +
> + if (!IS_ERR_OR_NULL(bus->buf_base)) {
> + bus->buf_size = resource_size(res);
> + if (of_device_is_compatible(pdev->dev.of_node,
> + "aspeed,ast2400-i2c-bus")) {
> + bus->buf_page = ((res->start >> 8) &
> + GENMASK(3, 0)) - 8;
> + bus->buf_offset = (res->start >> 2) &
> + ASPEED_I2CD_BUF_OFFSET_MASK;
> + }
> + }
> + }
> +
> /* Initialize the I2C adapter */
> spin_lock_init(&bus->lock);
> init_completion(&bus->cmd_complete);
> @@ -1026,8 +1255,8 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, bus);
>
> - dev_info(bus->dev, "i2c bus %d registered, irq %d\n",
> - bus->adap.nr, irq);
> + dev_info(bus->dev, "i2c bus %d registered (%s mode), irq %d\n",
> + bus->adap.nr, bus->buf_base ? "buffer" : "byte", irq);
>
> return 0;
> }
> --
> 2.23.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/5] i2c: aspeed: fix master pending state handling
2019-10-07 23:13 ` [PATCH 3/5] i2c: aspeed: fix master pending state handling Jae Hyun Yoo
@ 2019-10-08 20:31 ` Brendan Higgins
2019-10-08 21:13 ` Jae Hyun Yoo
2019-10-08 22:00 ` Tao Ren
1 sibling, 1 reply; 22+ messages in thread
From: Brendan Higgins @ 2019-10-08 20:31 UTC (permalink / raw)
To: Jae Hyun Yoo
Cc: Mark Rutland, devicetree, linux-aspeed, Andrew Jeffery,
Benjamin Herrenschmidt, openbmc, Rob Herring, Joel Stanley,
Tao Ren, linux-arm-kernel, linux-i2c
On Mon, Oct 07, 2019 at 04:13:11PM -0700, Jae Hyun Yoo wrote:
> In case of master pending state, it should not trigger the master
> command because this H/W is sharing the same data buffer for slave
> and master operations, so this commit fixes the issue with making
> the master command triggering happen when the state goes to active
> state.
nit: Makes sense, but can you explain what might happen without your
change?
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
Thanks!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] i2c: aspeed: add buffer mode transfer support
2019-10-08 20:12 ` Brendan Higgins
@ 2019-10-08 21:10 ` Jae Hyun Yoo
2019-10-08 23:15 ` Brendan Higgins
0 siblings, 1 reply; 22+ messages in thread
From: Jae Hyun Yoo @ 2019-10-08 21:10 UTC (permalink / raw)
To: Brendan Higgins
Cc: Mark Rutland, devicetree, linux-aspeed, Andrew Jeffery,
Benjamin Herrenschmidt, openbmc, Rob Herring, Joel Stanley,
Tao Ren, linux-arm-kernel, linux-i2c
Hi Brendan,
On 10/8/2019 1:12 PM, Brendan Higgins wrote:
> On Mon, Oct 07, 2019 at 04:13:12PM -0700, Jae Hyun Yoo wrote:
>> Byte mode currently this driver uses makes lots of interrupt call
>
> nit: Drop "Byte mode".
'Byte mode' is one of modes which is described in the datasheet.
Would it be better if I change it like below?
"This driver uses byte mode that makes lots of interrupt call ..."
>> 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.
>
> nit: Please use imperative mood.
I used imperative mood in commit title. The commit message is okay as it
is.
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> Tested-by: Tao Ren <taoren@fb.com>
>> ---
>> drivers/i2c/busses/i2c-aspeed.c | 297 ++++++++++++++++++++++++++++----
>> 1 file changed, 263 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index 40f6cf98d32e..37d1a7fa2f87 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)
>> @@ -102,6 +110,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)
>> @@ -112,6 +122,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,
>> @@ -157,6 +174,11 @@ struct aspeed_i2c_bus {
>> int master_xfer_result;
>> /* Multi-master */
>> bool multi_master;
>> + /* Buffer mode */
>> + void __iomem *buf_base;
>> + size_t buf_size;
>> + u8 buf_offset;
>> + u8 buf_page;
>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> struct i2c_client *slave;
>> enum aspeed_i2c_slave_state slave_state;
>> @@ -238,6 +260,7 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>> {
>> u32 command, irq_handled = 0;
>> struct i2c_client *slave = bus->slave;
>> + int i, len;
>> u8 value;
>>
>> if (!slave)
>> @@ -260,7 +283,12 @@ 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;
>> + if (bus->buf_base &&
>> + bus->slave_state == ASPEED_I2C_SLAVE_WRITE_RECEIVED &&
>> + !(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))
>
> I think checking for the buf_base all over the place makes this really
> complicated and hard to read.
>
> It might be better to just split this out and have separate handlers
> based on what mode the driver is running in.
I think you're saying about splitting this irq handler out to:
aspeed_i2c_slave_byte_mode_irq()
aspeed_i2c_slave_buffer_mode_irq()
aspeed_i2c_slave_dma_mode_irq()
Yes, I can do like that but it will bring us two bad things:
1. It makes big chunks of duplicate code because most of interrupt
handling logic is the same.
2. If we are going to change something in irq routine, we need to
touch all irq routines if the change is commonly used.
I think, the way this patch uses is better.
>> + value = readb(bus->buf_base);
>> + else
>> + value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
>> /* Handle address frame. */
>> if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
>> if (value & 0x1)
>> @@ -275,6 +303,20 @@ 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) {
>> + 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));
>
> It looks like you have a lot of improvements in here unrelated to adding
> support for buffer mode.
>
> I really appreciate the improvements, but it makes it harder to
> understand what buffer features you are adding vs. what
> improvments/modernizations you are making.
>
> Can you split this commit up?
No, this isn't an improvement. This code will not be executed if
transfer mode is byte mode. This is added because data handling pattern
is different in buffer mode so the collected data in buffer mode should
be sent when it recieves RX_DONE.
>> + for (i = 0; i < len; i++) {
>> + value = readb(bus->buf_base + i);
>> + i2c_slave_event(slave,
>> + I2C_SLAVE_WRITE_RECEIVED,
>> + &value);
>> + }
>> + }
>> + }
>> irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
>> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> }
>> @@ -307,9 +349,36 @@ 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);
>> + 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);
>> + }
>> break;
>> case ASPEED_I2C_SLAVE_WRITE_RECEIVED:
>> i2c_slave_event(slave, I2C_SLAVE_WRITE_RECEIVED, &value);
>> + 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(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);
>> + }
>> break;
>> case ASPEED_I2C_SLAVE_STOP:
>> i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
>> @@ -335,6 +404,8 @@ 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);
>> + u8 wbuf[4];
>> + int len;
>>
>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> /*
>> @@ -353,12 +424,66 @@ 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 (bus->buf_base && !(msg->flags & I2C_M_RECV_LEN)) {
>> + command |= ASPEED_I2CD_RX_BUFF_ENABLE;
>> +
>> + if (msg->len > bus->buf_size) {
>> + len = bus->buf_size;
>> + } else {
>> + len = msg->len;
>> + command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
>> + }
>> +
>> + 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);
>> + } else {
>> + /* 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;
>> + }
>> + } else {
>> + if (bus->buf_base) {
>> + int i;
>> +
>> + command |= ASPEED_I2CD_TX_BUFF_ENABLE;
>> +
>> + if (msg->len + 1 > bus->buf_size)
>> + len = bus->buf_size;
>> + else
>> + len = msg->len + 1;
>> +
>> + /*
>> + * Yeah, it looks clumsy but byte writings on a remapped
>> + * I2C SRAM cause corruptions 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));
>> +
>> + bus->buf_index = len - 1;
>> +
>> + 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(slave_addr, bus->base + ASPEED_I2C_BYTE_BUF_REG);
>> + if (!(command & ASPEED_I2CD_TX_BUFF_ENABLE))
>> + writel(slave_addr, bus->base + ASPEED_I2C_BYTE_BUF_REG);
>> writel(command, bus->base + ASPEED_I2C_CMD_REG);
>> }
>>
>> @@ -398,7 +523,7 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>> u32 irq_handled = 0, command = 0;
>> struct i2c_msg *msg;
>> u8 recv_byte;
>> - int ret;
>> + int ret, len;
>>
>> if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
>> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> @@ -511,11 +636,43 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>> /* fall through */
>> case ASPEED_I2C_MASTER_TX_FIRST:
>> if (bus->buf_index < msg->len) {
>> + command = ASPEED_I2CD_M_TX_CMD;
>> +
>> + if (bus->buf_base) {
>> + 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));
>> +
>> + bus->buf_index += len;
>> +
>> + 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);
>> + } else {
>> + writel(msg->buf[bus->buf_index++],
>> + bus->base + ASPEED_I2C_BYTE_BUF_REG);
>> + }
>> + 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);
>> }
>> @@ -532,25 +689,56 @@ 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) {
>> - if (unlikely(recv_byte > I2C_SMBUS_BLOCK_MAX)) {
>> - bus->cmd_err = -EPROTO;
>> - aspeed_i2c_do_stop(bus);
>> - goto out_no_complete;
>> + if (bus->buf_base && !(msg->flags & I2C_M_RECV_LEN)) {
>> + 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;
>> +
>> + if (msg->flags & I2C_M_RECV_LEN) {
>> + 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->flags &= ~I2C_M_RECV_LEN;
>> }
>> - msg->len = recv_byte +
>> - ((msg->flags & I2C_CLIENT_PEC) ? 2 : 1);
>> - msg->flags &= ~I2C_M_RECV_LEN;
>> }
>>
>> 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;
>> + bus->master_state = ASPEED_I2C_MASTER_RX;
>> + if (bus->buf_base) {
>> + command |= ASPEED_I2CD_RX_BUFF_ENABLE;
>> +
>> + 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;
>> + }
>> +
>> + 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;
>> + }
>> writel(command, bus->base + ASPEED_I2C_CMD_REG);
>> } else {
>> aspeed_i2c_next_msg_or_stop(bus);
>> @@ -890,6 +1078,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
>> @@ -947,16 +1138,15 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
>> {
>> const struct of_device_id *match;
>> struct aspeed_i2c_bus *bus;
>> + bool sram_enabled = true;
>> 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);
>>
>> @@ -990,6 +1180,45 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
>> bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
>> match->data;
>>
>> + /*
>> + * Enable I2C SRAM in case of AST2500.
>> + * SRAM is enabled by default in AST2400 and AST2600.
>> + */
>
> This probe function is already pretty complicated as it is. Can we move
> this to a helper function (especially since it only applies to the
> 25xx)?
Okay, that would be better. I'll add this transfer mode setting logic
as a helper function.
>> + if (of_device_is_compatible(pdev->dev.of_node,
>> + "aspeed,ast2500-i2c-bus")) {
>> + struct regmap *gr_regmap = syscon_regmap_lookup_by_compatible("aspeed,ast2500-i2c-gr");
>
> So this memory is global, right? It is shared by all the busses?
Yes, this is global register area which can be shared by all busses.
> If I am reading this right, then I think we need to protect so that only
> one bus is accessing this memory at a time.
It will not be accessed at run time but only at probing time. Since we
don't use multi-threaded probing, we don't need to protect it.
Thanks,
Jae
>> + if (IS_ERR(gr_regmap))
>> + ret = PTR_ERR(gr_regmap);
>> + else
>> + ret = regmap_update_bits(gr_regmap,
>> + ASPEED_I2CG_GLOBAL_CTRL_REG,
>> + ASPEED_I2CG_SRAM_BUFFER_EN,
>> + ASPEED_I2CG_SRAM_BUFFER_EN);
>> +
>> + if (ret)
>> + sram_enabled = false;
>> + }
>> +
>> + if (sram_enabled) {
>> + struct resource *res = platform_get_resource(pdev,
>> + IORESOURCE_MEM, 1);
>> +
>> + if (res && resource_size(res) >= 2)
>> + bus->buf_base = devm_ioremap_resource(&pdev->dev, res);
>> +
>> + if (!IS_ERR_OR_NULL(bus->buf_base)) {
>> + bus->buf_size = resource_size(res);
>> + if (of_device_is_compatible(pdev->dev.of_node,
>> + "aspeed,ast2400-i2c-bus")) {
>> + bus->buf_page = ((res->start >> 8) &
>> + GENMASK(3, 0)) - 8;
>> + bus->buf_offset = (res->start >> 2) &
>> + ASPEED_I2CD_BUF_OFFSET_MASK;
>> + }
>> + }
>> + }
>> +
>> /* Initialize the I2C adapter */
>> spin_lock_init(&bus->lock);
>> init_completion(&bus->cmd_complete);
>> @@ -1026,8 +1255,8 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
>>
>> platform_set_drvdata(pdev, bus);
>>
>> - dev_info(bus->dev, "i2c bus %d registered, irq %d\n",
>> - bus->adap.nr, irq);
>> + dev_info(bus->dev, "i2c bus %d registered (%s mode), irq %d\n",
>> + bus->adap.nr, bus->buf_base ? "buffer" : "byte", irq);
>>
>> return 0;
>> }
>> --
>> 2.23.0
>>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/5] i2c: aspeed: fix master pending state handling
2019-10-08 20:31 ` Brendan Higgins
@ 2019-10-08 21:13 ` Jae Hyun Yoo
2019-10-08 21:54 ` Brendan Higgins
0 siblings, 1 reply; 22+ messages in thread
From: Jae Hyun Yoo @ 2019-10-08 21:13 UTC (permalink / raw)
To: Brendan Higgins
Cc: Mark Rutland, devicetree, linux-aspeed, Andrew Jeffery,
Benjamin Herrenschmidt, openbmc, Rob Herring, Joel Stanley,
Tao Ren, linux-arm-kernel, linux-i2c
On 10/8/2019 1:31 PM, Brendan Higgins wrote:
> On Mon, Oct 07, 2019 at 04:13:11PM -0700, Jae Hyun Yoo wrote:
>> In case of master pending state, it should not trigger the master
>> command because this H/W is sharing the same data buffer for slave
>> and master operations, so this commit fixes the issue with making
>> the master command triggering happen when the state goes to active
>> state.
>
> nit: Makes sense, but can you explain what might happen without your
> change?
If we don't use this fix, a master command could corrupt data in the
shared buffer if H/W is still on processing slave operation at the
timing.
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
Thanks a lot for your review!
-Jae
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/5] i2c: aspeed: fix master pending state handling
2019-10-08 21:13 ` Jae Hyun Yoo
@ 2019-10-08 21:54 ` Brendan Higgins
2019-10-08 22:55 ` Jae Hyun Yoo
2019-10-10 5:28 ` Joel Stanley
0 siblings, 2 replies; 22+ messages in thread
From: Brendan Higgins @ 2019-10-08 21:54 UTC (permalink / raw)
To: Jae Hyun Yoo, Joel Stanley, Wolfram Sang
Cc: Mark Rutland, devicetree, linux-aspeed, Andrew Jeffery,
Benjamin Herrenschmidt, OpenBMC Maillist, Rob Herring, linux-i2c,
Tao Ren, Linux ARM
On Tue, Oct 8, 2019 at 2:13 PM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> wrote:
>
> On 10/8/2019 1:31 PM, Brendan Higgins wrote:
> > On Mon, Oct 07, 2019 at 04:13:11PM -0700, Jae Hyun Yoo wrote:
> >> In case of master pending state, it should not trigger the master
> >> command because this H/W is sharing the same data buffer for slave
> >> and master operations, so this commit fixes the issue with making
> >> the master command triggering happen when the state goes to active
> >> state.
> >
> > nit: Makes sense, but can you explain what might happen without your
> > change?
>
> If we don't use this fix, a master command could corrupt data in the
> shared buffer if H/W is still on processing slave operation at the
> timing.
Right, can you add that to the commit message?
Is this trivially reproducible? We might want to submit this
separately as a bugfix.
Actually yeah, can you send this separately as a bugfix? I think we
might want to include this in 5.4.
Wolfram and Joel, what do you think?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/5] i2c: aspeed: fix master pending state handling
2019-10-07 23:13 ` [PATCH 3/5] i2c: aspeed: fix master pending state handling Jae Hyun Yoo
2019-10-08 20:31 ` Brendan Higgins
@ 2019-10-08 22:00 ` Tao Ren
2019-10-08 22:45 ` Jae Hyun Yoo
1 sibling, 1 reply; 22+ messages in thread
From: Tao Ren @ 2019-10-08 22:00 UTC (permalink / raw)
To: Jae Hyun Yoo, Brendan Higgins, Benjamin Herrenschmidt,
Joel Stanley, Rob Herring, Mark Rutland, Andrew Jeffery
Cc: devicetree, linux-aspeed, linux-i2c, linux-arm-kernel, openbmc
On 10/7/19 4:13 PM, Jae Hyun Yoo wrote:
> In case of master pending state, it should not trigger the master
> command because this H/W is sharing the same data buffer for slave
> and master operations, so this commit fixes the issue with making
> the master command triggering happen when the state goes to active
> state.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
> drivers/i2c/busses/i2c-aspeed.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index fa66951b05d0..40f6cf98d32e 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -336,18 +336,19 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
> struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
> u8 slave_addr = i2c_8bit_addr_from_msg(msg);
>
> - bus->master_state = ASPEED_I2C_MASTER_START;
> -
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> /*
> * If it's requested in the middle of a slave session, set the master
> * state to 'pending' then H/W will continue handling this master
> * command when the bus comes back to the idle state.
> */
> - if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
> + if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) {
> bus->master_state = ASPEED_I2C_MASTER_PENDING;
> + return;
> + }
> #endif /* CONFIG_I2C_SLAVE */
>
> + bus->master_state = ASPEED_I2C_MASTER_START;
> bus->buf_index = 0;
>
> if (msg->flags & I2C_M_RD) {
> @@ -432,7 +433,7 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
> if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
> goto out_no_complete;
>
> - bus->master_state = ASPEED_I2C_MASTER_START;
> + aspeed_i2c_do_start(bus);
> }
Shall we move the restart-master logic from master_irq to bus_irq? The reason being:
master transaction cannot be restarted when aspeed-i2c is running in slave state and
receives STOP interrupt, because aspeed_i2c_master_irq won't be called in this case.
Cheers,
Tao
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/5] i2c: aspeed: fix master pending state handling
2019-10-08 22:00 ` Tao Ren
@ 2019-10-08 22:45 ` Jae Hyun Yoo
2019-10-08 23:15 ` Tao Ren
0 siblings, 1 reply; 22+ messages in thread
From: Jae Hyun Yoo @ 2019-10-08 22:45 UTC (permalink / raw)
To: Tao Ren, Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Rob Herring, Mark Rutland, Andrew Jeffery
Cc: devicetree, linux-aspeed, linux-i2c, linux-arm-kernel, openbmc
Hi Tao,
On 10/8/2019 3:00 PM, Tao Ren wrote:
> On 10/7/19 4:13 PM, Jae Hyun Yoo wrote:
>> In case of master pending state, it should not trigger the master
>> command because this H/W is sharing the same data buffer for slave
>> and master operations, so this commit fixes the issue with making
>> the master command triggering happen when the state goes to active
>> state.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>> drivers/i2c/busses/i2c-aspeed.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index fa66951b05d0..40f6cf98d32e 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -336,18 +336,19 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
>> struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
>> u8 slave_addr = i2c_8bit_addr_from_msg(msg);
>>
>> - bus->master_state = ASPEED_I2C_MASTER_START;
>> -
>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> /*
>> * If it's requested in the middle of a slave session, set the master
>> * state to 'pending' then H/W will continue handling this master
>> * command when the bus comes back to the idle state.
>> */
>> - if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
>> + if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) {
>> bus->master_state = ASPEED_I2C_MASTER_PENDING;
>> + return;
>> + }
>> #endif /* CONFIG_I2C_SLAVE */
>>
>> + bus->master_state = ASPEED_I2C_MASTER_START;
>> bus->buf_index = 0;
>>
>> if (msg->flags & I2C_M_RD) {
>> @@ -432,7 +433,7 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>> if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
>> goto out_no_complete;
>>
>> - bus->master_state = ASPEED_I2C_MASTER_START;
>> + aspeed_i2c_do_start(bus);
>> }
>
> Shall we move the restart-master logic from master_irq to bus_irq? The reason being:
> master transaction cannot be restarted when aspeed-i2c is running in slave state and
> receives STOP interrupt, because aspeed_i2c_master_irq won't be called in this case.
Even in that case, master can be restarted properly because slave_irq
will be called first because master is in MASTER_PENDING state, so the
slave_irq handles the STOP interrupt as well, and then master_irq will
be called with SLAVE_INACTIVE state so the aspeed_i2c_do_start can be
called eventually.
Also, this is right point to call the aspeed_i2c_do_start
because master state will be changed to MASTER_START by the
aspeed_i2c_do_start and we have to do remaining handling for the
MASTER_START in the master_irq by falling through after the call.
Thanks,
Jae
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/5] i2c: aspeed: fix master pending state handling
2019-10-08 21:54 ` Brendan Higgins
@ 2019-10-08 22:55 ` Jae Hyun Yoo
2019-10-10 5:28 ` Joel Stanley
1 sibling, 0 replies; 22+ messages in thread
From: Jae Hyun Yoo @ 2019-10-08 22:55 UTC (permalink / raw)
To: Brendan Higgins, Joel Stanley, Wolfram Sang
Cc: Mark Rutland, devicetree, linux-aspeed, Andrew Jeffery,
Benjamin Herrenschmidt, OpenBMC Maillist, Rob Herring, linux-i2c,
Tao Ren, Linux ARM
On 10/8/2019 2:54 PM, Brendan Higgins wrote:
> On Tue, Oct 8, 2019 at 2:13 PM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
>>
>> On 10/8/2019 1:31 PM, Brendan Higgins wrote:
>>> On Mon, Oct 07, 2019 at 04:13:11PM -0700, Jae Hyun Yoo wrote:
>>>> In case of master pending state, it should not trigger the master
>>>> command because this H/W is sharing the same data buffer for slave
>>>> and master operations, so this commit fixes the issue with making
>>>> the master command triggering happen when the state goes to active
>>>> state.
>>>
>>> nit: Makes sense, but can you explain what might happen without your
>>> change?
>>
>> If we don't use this fix, a master command could corrupt data in the
>> shared buffer if H/W is still on processing slave operation at the
>> timing.
>
> Right, can you add that to the commit message?
Sure, will do that.
> Is this trivially reproducible? We might want to submit this
> separately as a bugfix.
It's very rarely observed.
> Actually yeah, can you send this separately as a bugfix? I think we
> might want to include this in 5.4.
Why not. I can send it separately but this patch series should wait for
merging the bug fix to avoid context conflicts.
> Wolfram and Joel, what do you think?
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] i2c: aspeed: add buffer mode transfer support
2019-10-08 21:10 ` Jae Hyun Yoo
@ 2019-10-08 23:15 ` Brendan Higgins
2019-10-09 0:08 ` Jae Hyun Yoo
0 siblings, 1 reply; 22+ messages in thread
From: Brendan Higgins @ 2019-10-08 23:15 UTC (permalink / raw)
To: Jae Hyun Yoo
Cc: Mark Rutland, devicetree, linux-aspeed, Andrew Jeffery,
Benjamin Herrenschmidt, OpenBMC Maillist, Rob Herring,
Joel Stanley, Tao Ren, Linux ARM, linux-i2c
On Tue, Oct 8, 2019 at 2:10 PM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> wrote:
>
> Hi Brendan,
>
> On 10/8/2019 1:12 PM, Brendan Higgins wrote:
> > On Mon, Oct 07, 2019 at 04:13:12PM -0700, Jae Hyun Yoo wrote:
> >> Byte mode currently this driver uses makes lots of interrupt call
> >
> > nit: Drop "Byte mode".
>
> 'Byte mode' is one of modes which is described in the datasheet.
>
> Would it be better if I change it like below?
> "This driver uses byte mode that makes lots of interrupt call ..."
Yeah, I think that would probably be clearer.
> >> 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.
> >
> > nit: Please use imperative mood.
>
> I used imperative mood in commit title. The commit message is okay as it
> is.
Hey, that's just what I have been told in the past. I don't actually
feel strongly about it though. If no one else cares, then it is fine.
> >> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> >> Tested-by: Tao Ren <taoren@fb.com>
> >> ---
> >> drivers/i2c/busses/i2c-aspeed.c | 297 ++++++++++++++++++++++++++++----
> >> 1 file changed, 263 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> >> index 40f6cf98d32e..37d1a7fa2f87 100644
> >> --- a/drivers/i2c/busses/i2c-aspeed.c
> >> +++ b/drivers/i2c/busses/i2c-aspeed.c
[...]
> >> @@ -238,6 +260,7 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
> >> {
> >> u32 command, irq_handled = 0;
> >> struct i2c_client *slave = bus->slave;
> >> + int i, len;
> >> u8 value;
> >>
> >> if (!slave)
> >> @@ -260,7 +283,12 @@ 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;
> >> + if (bus->buf_base &&
> >> + bus->slave_state == ASPEED_I2C_SLAVE_WRITE_RECEIVED &&
> >> + !(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))
> >
> > I think checking for the buf_base all over the place makes this really
> > complicated and hard to read.
> >
> > It might be better to just split this out and have separate handlers
> > based on what mode the driver is running in.
>
> I think you're saying about splitting this irq handler out to:
> aspeed_i2c_slave_byte_mode_irq()
> aspeed_i2c_slave_buffer_mode_irq()
> aspeed_i2c_slave_dma_mode_irq()
>
> Yes, I can do like that but it will bring us two bad things:
> 1. It makes big chunks of duplicate code because most of interrupt
> handling logic is the same.
> 2. If we are going to change something in irq routine, we need to
> touch all irq routines if the change is commonly used.
>
> I think, the way this patch uses is better.
I think there are other alternatives. For example, I think you could
abstract over the buffer reading mechanism here.
We might have a method on aspeed_i2c_bus called handle_rx_done() or
something like that which could get called here.
I just really don't want to grow the McCabe's complexity of this
function much more, it is really too high as it is. Nevertheless, I am
open to other suggestions on how to improve this function.
> >> + value = readb(bus->buf_base);
> >> + else
> >> + value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
> >> /* Handle address frame. */
> >> if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
> >> if (value & 0x1)
> >> @@ -275,6 +303,20 @@ 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) {
> >> + 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));
> >
> > It looks like you have a lot of improvements in here unrelated to adding
> > support for buffer mode.
> >
> > I really appreciate the improvements, but it makes it harder to
> > understand what buffer features you are adding vs. what
> > improvments/modernizations you are making.
> >
> > Can you split this commit up?
>
> No, this isn't an improvement. This code will not be executed if
> transfer mode is byte mode. This is added because data handling pattern
> is different in buffer mode so the collected data in buffer mode should
> be sent when it recieves RX_DONE.
Oh sorry about that, I saw the switch to the
devm_platform_ioremap_resource below and saw all the FIELD_{GET|PREP}
and assumed that some of them were improvements. If
devm_platform_ioremap_resource is the only one, that's fine.
Actually, would you mind (in a separate commit), update the existing
usages to FIELD_{GET|PREP}? It's kind of jarring going back and forth
between them.
> >> + for (i = 0; i < len; i++) {
> >> + value = readb(bus->buf_base + i);
> >> + i2c_slave_event(slave,
> >> + I2C_SLAVE_WRITE_RECEIVED,
> >> + &value);
> >> + }
> >> + }
> >> + }
> >> irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
> >> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> >> }
[....]
> >> @@ -990,6 +1180,45 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
> >> bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
> >> match->data;
> >>
> >> + /*
> >> + * Enable I2C SRAM in case of AST2500.
> >> + * SRAM is enabled by default in AST2400 and AST2600.
> >> + */
> >
> > This probe function is already pretty complicated as it is. Can we move
> > this to a helper function (especially since it only applies to the
> > 25xx)?
>
> Okay, that would be better. I'll add this transfer mode setting logic
> as a helper function.
>
> >> + if (of_device_is_compatible(pdev->dev.of_node,
> >> + "aspeed,ast2500-i2c-bus")) {
> >> + struct regmap *gr_regmap = syscon_regmap_lookup_by_compatible("aspeed,ast2500-i2c-gr");
> >
> > So this memory is global, right? It is shared by all the busses?
>
> Yes, this is global register area which can be shared by all busses.
>
> > If I am reading this right, then I think we need to protect so that only
> > one bus is accessing this memory at a time.
>
> It will not be accessed at run time but only at probing time. Since we
> don't use multi-threaded probing, we don't need to protect it.
What if this is loaded as a module?
Also, it seems as though turning on SRAM should only happen once. Is
this correct?
> >> + if (IS_ERR(gr_regmap))
> >> + ret = PTR_ERR(gr_regmap);
> >> + else
> >> + ret = regmap_update_bits(gr_regmap,
> >> + ASPEED_I2CG_GLOBAL_CTRL_REG,
> >> + ASPEED_I2CG_SRAM_BUFFER_EN,
> >> + ASPEED_I2CG_SRAM_BUFFER_EN);
> >> +
> >> + if (ret)
> >> + sram_enabled = false;
> >> + }
> >> +
> >> + if (sram_enabled) {
> >> + struct resource *res = platform_get_resource(pdev,
> >> + IORESOURCE_MEM, 1);
> >> +
> >> + if (res && resource_size(res) >= 2)
> >> + bus->buf_base = devm_ioremap_resource(&pdev->dev, res);
> >> +
> >> + if (!IS_ERR_OR_NULL(bus->buf_base)) {
> >> + bus->buf_size = resource_size(res);
> >> + if (of_device_is_compatible(pdev->dev.of_node,
> >> + "aspeed,ast2400-i2c-bus")) {
> >> + bus->buf_page = ((res->start >> 8) &
> >> + GENMASK(3, 0)) - 8;
> >> + bus->buf_offset = (res->start >> 2) &
> >> + ASPEED_I2CD_BUF_OFFSET_MASK;
> >> + }
> >> + }
> >> + }
[...]
Cheers
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/5] i2c: aspeed: fix master pending state handling
2019-10-08 22:45 ` Jae Hyun Yoo
@ 2019-10-08 23:15 ` Tao Ren
2019-10-08 23:28 ` Jae Hyun Yoo
0 siblings, 1 reply; 22+ messages in thread
From: Tao Ren @ 2019-10-08 23:15 UTC (permalink / raw)
To: Jae Hyun Yoo, Brendan Higgins, Benjamin Herrenschmidt,
Joel Stanley, Rob Herring, Mark Rutland, Andrew Jeffery
Cc: devicetree, linux-aspeed, linux-i2c, linux-arm-kernel, openbmc
On 10/8/19 3:45 PM, Jae Hyun Yoo wrote:
> Hi Tao,
>
> On 10/8/2019 3:00 PM, Tao Ren wrote:
>> On 10/7/19 4:13 PM, Jae Hyun Yoo wrote:
>>> In case of master pending state, it should not trigger the master
>>> command because this H/W is sharing the same data buffer for slave
>>> and master operations, so this commit fixes the issue with making
>>> the master command triggering happen when the state goes to active
>>> state.
>>>
>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>>> ---
>>> drivers/i2c/busses/i2c-aspeed.c | 9 +++++----
>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>> index fa66951b05d0..40f6cf98d32e 100644
>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>> @@ -336,18 +336,19 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
>>> struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
>>> u8 slave_addr = i2c_8bit_addr_from_msg(msg);
>>> - bus->master_state = ASPEED_I2C_MASTER_START;
>>> -
>>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>> /*
>>> * If it's requested in the middle of a slave session, set the master
>>> * state to 'pending' then H/W will continue handling this master
>>> * command when the bus comes back to the idle state.
>>> */
>>> - if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
>>> + if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) {
>>> bus->master_state = ASPEED_I2C_MASTER_PENDING;
>>> + return;
>>> + }
>>> #endif /* CONFIG_I2C_SLAVE */
>>> + bus->master_state = ASPEED_I2C_MASTER_START;
>>> bus->buf_index = 0;
>>> if (msg->flags & I2C_M_RD) {
>>> @@ -432,7 +433,7 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>>> if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
>>> goto out_no_complete;
>>> - bus->master_state = ASPEED_I2C_MASTER_START;
>>> + aspeed_i2c_do_start(bus);
>>> }
>>
>> Shall we move the restart-master logic from master_irq to bus_irq? The reason being:
>> master transaction cannot be restarted when aspeed-i2c is running in slave state and
>> receives STOP interrupt, because aspeed_i2c_master_irq won't be called in this case.
>
> Even in that case, master can be restarted properly because slave_irq
> will be called first because master is in MASTER_PENDING state, so the
> slave_irq handles the STOP interrupt as well, and then master_irq will
> be called with SLAVE_INACTIVE state so the aspeed_i2c_do_start can be
> called eventually.
I mean master_irq cannot be called when irq_remaining becomes 0 after slave_irq.
Cheers,
Tao
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/5] i2c: aspeed: fix master pending state handling
2019-10-08 23:15 ` Tao Ren
@ 2019-10-08 23:28 ` Jae Hyun Yoo
0 siblings, 0 replies; 22+ messages in thread
From: Jae Hyun Yoo @ 2019-10-08 23:28 UTC (permalink / raw)
To: Tao Ren, Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Rob Herring, Mark Rutland, Andrew Jeffery
Cc: devicetree, linux-aspeed, linux-i2c, linux-arm-kernel, openbmc
On 10/8/2019 4:15 PM, Tao Ren wrote:
> On 10/8/19 3:45 PM, Jae Hyun Yoo wrote:
>> Hi Tao,
>>
>> On 10/8/2019 3:00 PM, Tao Ren wrote:
>>> On 10/7/19 4:13 PM, Jae Hyun Yoo wrote:
>>>> In case of master pending state, it should not trigger the master
>>>> command because this H/W is sharing the same data buffer for slave
>>>> and master operations, so this commit fixes the issue with making
>>>> the master command triggering happen when the state goes to active
>>>> state.
>>>>
>>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>>>> ---
>>>> drivers/i2c/busses/i2c-aspeed.c | 9 +++++----
>>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>>> index fa66951b05d0..40f6cf98d32e 100644
>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>> @@ -336,18 +336,19 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus)
>>>> struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
>>>> u8 slave_addr = i2c_8bit_addr_from_msg(msg);
>>>> - bus->master_state = ASPEED_I2C_MASTER_START;
>>>> -
>>>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>> /*
>>>> * If it's requested in the middle of a slave session, set the master
>>>> * state to 'pending' then H/W will continue handling this master
>>>> * command when the bus comes back to the idle state.
>>>> */
>>>> - if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
>>>> + if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE) {
>>>> bus->master_state = ASPEED_I2C_MASTER_PENDING;
>>>> + return;
>>>> + }
>>>> #endif /* CONFIG_I2C_SLAVE */
>>>> + bus->master_state = ASPEED_I2C_MASTER_START;
>>>> bus->buf_index = 0;
>>>> if (msg->flags & I2C_M_RD) {
>>>> @@ -432,7 +433,7 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>>>> if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE)
>>>> goto out_no_complete;
>>>> - bus->master_state = ASPEED_I2C_MASTER_START;
>>>> + aspeed_i2c_do_start(bus);
>>>> }
>>>
>>> Shall we move the restart-master logic from master_irq to bus_irq? The reason being:
>>> master transaction cannot be restarted when aspeed-i2c is running in slave state and
>>> receives STOP interrupt, because aspeed_i2c_master_irq won't be called in this case.
>>
>> Even in that case, master can be restarted properly because slave_irq
>> will be called first because master is in MASTER_PENDING state, so the
>> slave_irq handles the STOP interrupt as well, and then master_irq will
>> be called with SLAVE_INACTIVE state so the aspeed_i2c_do_start can be
>> called eventually.
>
> I mean master_irq cannot be called when irq_remaining becomes 0 after slave_irq.
Ah, I see. It would be possibly happened. Probably we need to remove
'if (irq_remaining)' checking in bus_irq to make it call irqs always.
Will fix the issue in the next round.
Thanks,
Jae
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] i2c: aspeed: add buffer mode transfer support
2019-10-08 23:15 ` Brendan Higgins
@ 2019-10-09 0:08 ` Jae Hyun Yoo
0 siblings, 0 replies; 22+ messages in thread
From: Jae Hyun Yoo @ 2019-10-09 0:08 UTC (permalink / raw)
To: Brendan Higgins
Cc: Mark Rutland, devicetree, linux-aspeed, Andrew Jeffery,
Benjamin Herrenschmidt, OpenBMC Maillist, Rob Herring,
Joel Stanley, Tao Ren, Linux ARM, linux-i2c
On 10/8/2019 4:15 PM, Brendan Higgins wrote:
> On Tue, Oct 8, 2019 at 2:10 PM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
>>
>> Hi Brendan,
>>
>> On 10/8/2019 1:12 PM, Brendan Higgins wrote:
>>> On Mon, Oct 07, 2019 at 04:13:12PM -0700, Jae Hyun Yoo wrote:
>>>> Byte mode currently this driver uses makes lots of interrupt call
>>>
>>> nit: Drop "Byte mode".
>>
>> 'Byte mode' is one of modes which is described in the datasheet.
>>
>> Would it be better if I change it like below?
>> "This driver uses byte mode that makes lots of interrupt call ..."
>
> Yeah, I think that would probably be clearer.
Okay. Will change it.
>>>> 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.
>>>
>>> nit: Please use imperative mood.
>>
>> I used imperative mood in commit title. The commit message is okay as it
>> is.
>
> Hey, that's just what I have been told in the past. I don't actually
> feel strongly about it though. If no one else cares, then it is fine.
Yeah, I suggested that in the past on a commit title of your patch not
on a commit message body. Anyway, it's not a strict rule.
>>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>>>> Tested-by: Tao Ren <taoren@fb.com>
>>>> ---
>>>> drivers/i2c/busses/i2c-aspeed.c | 297 ++++++++++++++++++++++++++++----
>>>> 1 file changed, 263 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>>> index 40f6cf98d32e..37d1a7fa2f87 100644
>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
> [...]
>>>> @@ -238,6 +260,7 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>>>> {
>>>> u32 command, irq_handled = 0;
>>>> struct i2c_client *slave = bus->slave;
>>>> + int i, len;
>>>> u8 value;
>>>>
>>>> if (!slave)
>>>> @@ -260,7 +283,12 @@ 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;
>>>> + if (bus->buf_base &&
>>>> + bus->slave_state == ASPEED_I2C_SLAVE_WRITE_RECEIVED &&
>>>> + !(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))
>>>
>>> I think checking for the buf_base all over the place makes this really
>>> complicated and hard to read.
>>>
>>> It might be better to just split this out and have separate handlers
>>> based on what mode the driver is running in.
>>
>> I think you're saying about splitting this irq handler out to:
>> aspeed_i2c_slave_byte_mode_irq()
>> aspeed_i2c_slave_buffer_mode_irq()
>> aspeed_i2c_slave_dma_mode_irq()
>>
>> Yes, I can do like that but it will bring us two bad things:
>> 1. It makes big chunks of duplicate code because most of interrupt
>> handling logic is the same.
>> 2. If we are going to change something in irq routine, we need to
>> touch all irq routines if the change is commonly used.
>>
>> I think, the way this patch uses is better.
>
> I think there are other alternatives. For example, I think you could
> abstract over the buffer reading mechanism here.
>
> We might have a method on aspeed_i2c_bus called handle_rx_done() or
> something like that which could get called here.
>
> I just really don't want to grow the McCabe's complexity of this
> function much more, it is really too high as it is. Nevertheless, I am
> open to other suggestions on how to improve this function.
Okay, Using of abstract functions would be a better way to simplify it.
Will update it in the next spin.
>>>> + value = readb(bus->buf_base);
>>>> + else
>>>> + value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
>>>> /* Handle address frame. */
>>>> if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
>>>> if (value & 0x1)
>>>> @@ -275,6 +303,20 @@ 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) {
>>>> + 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));
>>>
>>> It looks like you have a lot of improvements in here unrelated to adding
>>> support for buffer mode.
>>>
>>> I really appreciate the improvements, but it makes it harder to
>>> understand what buffer features you are adding vs. what
>>> improvments/modernizations you are making.
>>>
>>> Can you split this commit up?
>>
>> No, this isn't an improvement. This code will not be executed if
>> transfer mode is byte mode. This is added because data handling pattern
>> is different in buffer mode so the collected data in buffer mode should
>> be sent when it recieves RX_DONE.
>
> Oh sorry about that, I saw the switch to the
> devm_platform_ioremap_resource below and saw all the FIELD_{GET|PREP}
> and assumed that some of them were improvements. If
> devm_platform_ioremap_resource is the only one, that's fine.
>
> Actually, would you mind (in a separate commit), update the existing
> usages to FIELD_{GET|PREP}? It's kind of jarring going back and forth
> between them.
No. Will do that later using a separate commit.
>>>> + for (i = 0; i < len; i++) {
>>>> + value = readb(bus->buf_base + i);
>>>> + i2c_slave_event(slave,
>>>> + I2C_SLAVE_WRITE_RECEIVED,
>>>> + &value);
>>>> + }
>>>> + }
>>>> + }
>>>> irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
>>>> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>>>> }
> [....]
>>>> @@ -990,6 +1180,45 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
>>>> bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
>>>> match->data;
>>>>
>>>> + /*
>>>> + * Enable I2C SRAM in case of AST2500.
>>>> + * SRAM is enabled by default in AST2400 and AST2600.
>>>> + */
>>>
>>> This probe function is already pretty complicated as it is. Can we move
>>> this to a helper function (especially since it only applies to the
>>> 25xx)?
>>
>> Okay, that would be better. I'll add this transfer mode setting logic
>> as a helper function.
>>
>>>> + if (of_device_is_compatible(pdev->dev.of_node,
>>>> + "aspeed,ast2500-i2c-bus")) {
>>>> + struct regmap *gr_regmap = syscon_regmap_lookup_by_compatible("aspeed,ast2500-i2c-gr");
>>>
>>> So this memory is global, right? It is shared by all the busses?
>>
>> Yes, this is global register area which can be shared by all busses.
>>
>>> If I am reading this right, then I think we need to protect so that only
>>> one bus is accessing this memory at a time.
>>
>> It will not be accessed at run time but only at probing time. Since we
>> don't use multi-threaded probing, we don't need to protect it.
>
> What if this is loaded as a module?
Loading modules at the same time? This driver just enables the bit. It
doesn't have a bit clearing code so it would be safe even in that case.
Actually, I2C SRAM doesn't need to be disabled, means that the SRAM can
be left as enabled for all xfer modes. It's an unnecessary control so
Aspeed removed this register setting in AST2600 after enabling the SRAM
always by default.
> Also, it seems as though turning on SRAM should only happen once. Is
> this correct?
It's a global setting which affects all busses so it can be set just
once. Since we don't need to clear the bit, each bus driver enables the
bit without checking the bit that is simple way.
Thanks,
Jae
>>>> + if (IS_ERR(gr_regmap))
>>>> + ret = PTR_ERR(gr_regmap);
>>>> + else
>>>> + ret = regmap_update_bits(gr_regmap,
>>>> + ASPEED_I2CG_GLOBAL_CTRL_REG,
>>>> + ASPEED_I2CG_SRAM_BUFFER_EN,
>>>> + ASPEED_I2CG_SRAM_BUFFER_EN);
>>>> +
>>>> + if (ret)
>>>> + sram_enabled = false;
>>>> + }
>>>> +
>>>> + if (sram_enabled) {
>>>> + struct resource *res = platform_get_resource(pdev,
>>>> + IORESOURCE_MEM, 1);
>>>> +
>>>> + if (res && resource_size(res) >= 2)
>>>> + bus->buf_base = devm_ioremap_resource(&pdev->dev, res);
>>>> +
>>>> + if (!IS_ERR_OR_NULL(bus->buf_base)) {
>>>> + bus->buf_size = resource_size(res);
>>>> + if (of_device_is_compatible(pdev->dev.of_node,
>>>> + "aspeed,ast2400-i2c-bus")) {
>>>> + bus->buf_page = ((res->start >> 8) &
>>>> + GENMASK(3, 0)) - 8;
>>>> + bus->buf_offset = (res->start >> 2) &
>>>> + ASPEED_I2CD_BUF_OFFSET_MASK;
>>>> + }
>>>> + }
>>>> + }
> [...]
>
> Cheers
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/5] ARM: dts: aspeed: add I2C buffer mode support
2019-10-07 23:13 ` [PATCH 2/5] ARM: dts: aspeed: add I2C buffer mode support Jae Hyun Yoo
@ 2019-10-09 13:32 ` Cédric Le Goater
0 siblings, 0 replies; 22+ messages in thread
From: Cédric Le Goater @ 2019-10-09 13:32 UTC (permalink / raw)
To: Jae Hyun Yoo, Brendan Higgins, Benjamin Herrenschmidt,
Joel Stanley, Rob Herring, Mark Rutland, Andrew Jeffery, Tao Ren
Cc: linux-arm-kernel, devicetree, openbmc, linux-i2c, linux-aspeed
On 08/10/2019 01:13, Jae Hyun Yoo wrote:
> Byte mode currently this driver uses makes lots of interrupt call
> 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.
>
> 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.
>
> This commit fixes all I2C bus nodes to support buffer mode
> transfer.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> arch/arm/boot/dts/aspeed-g4.dtsi | 47 +++++++++++++++++++-------------
> arch/arm/boot/dts/aspeed-g5.dtsi | 47 +++++++++++++++++++-------------
> 2 files changed, 56 insertions(+), 38 deletions(-)
>
> diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
> index dffb595d30e4..f51b016aa769 100644
> --- a/arch/arm/boot/dts/aspeed-g4.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g4.dtsi
> @@ -420,12 +420,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 {
> @@ -433,7 +442,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>;
> @@ -449,7 +458,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>;
> @@ -465,7 +474,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>;
> @@ -482,7 +491,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>;
> @@ -499,7 +508,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>;
> @@ -516,7 +525,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>;
> @@ -533,7 +542,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>;
> @@ -550,7 +559,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>;
> @@ -567,7 +576,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>;
> @@ -584,7 +593,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>;
> @@ -601,7 +610,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>;
> @@ -618,7 +627,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>;
> @@ -635,7 +644,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>;
> @@ -652,7 +661,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 e8feb8b66a2f..cbc31ce3fab2 100644
> --- a/arch/arm/boot/dts/aspeed-g5.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g5.dtsi
> @@ -513,12 +513,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 {
> @@ -526,7 +535,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>;
> @@ -542,7 +551,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>;
> @@ -558,7 +567,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>;
> @@ -575,7 +584,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>;
> @@ -592,7 +601,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>;
> @@ -609,7 +618,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>;
> @@ -626,7 +635,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>;
> @@ -643,7 +652,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>;
> @@ -660,7 +669,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>;
> @@ -677,7 +686,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>;
> @@ -694,7 +703,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>;
> @@ -711,7 +720,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>;
> @@ -728,7 +737,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>;
> @@ -745,7 +754,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>;
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/5] i2c: aspeed: fix master pending state handling
2019-10-08 21:54 ` Brendan Higgins
2019-10-08 22:55 ` Jae Hyun Yoo
@ 2019-10-10 5:28 ` Joel Stanley
1 sibling, 0 replies; 22+ messages in thread
From: Joel Stanley @ 2019-10-10 5:28 UTC (permalink / raw)
To: Brendan Higgins
Cc: Mark Rutland, devicetree, Jae Hyun Yoo, linux-aspeed,
Wolfram Sang, Andrew Jeffery, Benjamin Herrenschmidt,
OpenBMC Maillist, Rob Herring, linux-i2c, Tao Ren, Linux ARM
On Tue, 8 Oct 2019 at 21:54, Brendan Higgins <brendanhiggins@google.com> wrote:
>
> On Tue, Oct 8, 2019 at 2:13 PM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
> >
> > On 10/8/2019 1:31 PM, Brendan Higgins wrote:
> > > On Mon, Oct 07, 2019 at 04:13:11PM -0700, Jae Hyun Yoo wrote:
> > >> In case of master pending state, it should not trigger the master
> > >> command because this H/W is sharing the same data buffer for slave
> > >> and master operations, so this commit fixes the issue with making
> > >> the master command triggering happen when the state goes to active
> > >> state.
> > >
> > > nit: Makes sense, but can you explain what might happen without your
> > > change?
> >
> > If we don't use this fix, a master command could corrupt data in the
> > shared buffer if H/W is still on processing slave operation at the
> > timing.
>
> Right, can you add that to the commit message?
>
> Is this trivially reproducible? We might want to submit this
> separately as a bugfix.
>
> Actually yeah, can you send this separately as a bugfix? I think we
> might want to include this in 5.4.
>
> Wolfram and Joel, what do you think?
Yes, good suggestion. A corruption fix should be merged I think.
Always send bug fixes upstream with Fixes tags so they land in the
stable tree. This is preferable to sending them separately to the
openbmc for inclusion in that tree, and potentially reaches a wider
audience.
Cheers,
Joel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-10-10 5:28 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-07 23:13 [PATCH 0/5] i2c: aspeed: Add buffer and DMA modes support Jae Hyun Yoo
2019-10-07 23:13 ` [PATCH 1/5] dt-bindings: i2c: aspeed: add buffer and DMA mode transfer support Jae Hyun Yoo
2019-10-08 18:12 ` Brendan Higgins
2019-10-08 18:47 ` Jae Hyun Yoo
2019-10-07 23:13 ` [PATCH 2/5] ARM: dts: aspeed: add I2C buffer mode support Jae Hyun Yoo
2019-10-09 13:32 ` Cédric Le Goater
2019-10-07 23:13 ` [PATCH 3/5] i2c: aspeed: fix master pending state handling Jae Hyun Yoo
2019-10-08 20:31 ` Brendan Higgins
2019-10-08 21:13 ` Jae Hyun Yoo
2019-10-08 21:54 ` Brendan Higgins
2019-10-08 22:55 ` Jae Hyun Yoo
2019-10-10 5:28 ` Joel Stanley
2019-10-08 22:00 ` Tao Ren
2019-10-08 22:45 ` Jae Hyun Yoo
2019-10-08 23:15 ` Tao Ren
2019-10-08 23:28 ` Jae Hyun Yoo
2019-10-07 23:13 ` [PATCH 4/5] i2c: aspeed: add buffer mode transfer support Jae Hyun Yoo
2019-10-08 20:12 ` Brendan Higgins
2019-10-08 21:10 ` Jae Hyun Yoo
2019-10-08 23:15 ` Brendan Higgins
2019-10-09 0:08 ` Jae Hyun Yoo
2019-10-07 23:13 ` [PATCH 5/5] i2c: aspeed: add DMA " Jae Hyun Yoo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).