linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/2] Add ASPEED AST2600 I2Cv2 controller driver
@ 2023-02-26  3:13 Ryan Chen
  2023-02-26  3:13 ` [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2 Ryan Chen
  2023-02-26  3:13 ` [PATCH v6 2/2] i2c: aspeed: support ast2600 i2cv new register mode driver Ryan Chen
  0 siblings, 2 replies; 28+ messages in thread
From: Ryan Chen @ 2023-02-26  3:13 UTC (permalink / raw)
  To: Ryan Chen, Andrew Jeffery, Brendan Higgins,
	Benjamin Herrenschmidt, Joel Stanley, Rob Herring,
	Krzysztof Kozlowski, Philipp Zabel, linux-i2c, openbmc,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel

This series add AST2600 i2cv2 new register set driver. The i2cv2 new
register set have new clock divider option for more flexiable generation.
And also have separate i2c master and slave register set for control.

The legacy register layout is mix master/slave register control together.
The following is add more detail description about new register layout.
And new feature set add for register.

-Add new clock divider option for more flexible and accurate clock
rate generation
-Add tCKHighMin timing to guarantee SCL high pulse width.
-Add support dual pool buffer mode, split 32 bytes pool buffer of
each device into 2 x 16 bytes for Tx and Rx individually.
-Increase DMA buffer size to 4096 bytes and support byte alignment.
-Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
-Re-define registers for separating master and slave mode control.
-Support 4 individual DMA buffers for master Tx and Rx, slave Tx and Rx.

And following is new register set for package transfer sequence.
-New Master operation mode:
 S -> Aw -> P
 S -> Aw -> TxD -> P
 S -> Ar -> RxD -> P
 S -> Aw -> RxD -> Sr -> Ar -> TxD -> P
-Bus SDA lock auto-release capability for new master DMA command mode.
-Bus auto timeout for new master/slave DMA mode.

The following is two versus register layout.
Old:
{I2CD00}: Function Control Register     
{I2CD04}: Clock and AC Timing Control Register
{I2CD08}: Clock and AC Timing Control Register
{I2CD0C}: Interrupt Control Register
{I2CD10}: Interrupt Status Register 
{I2CD14}: Command/Status Register   
{I2CD18}: Slave Device Address Register
{I2CD1C}: Pool Buffer Control Register
{I2CD20}: Transmit/Receive Byte Buffer Register
{I2CD24}: DMA Mode Buffer Address Register
{I2CD28}: DMA Transfer Length Register
{I2CD2C}: Original DMA Mode Buffer Address Setting
{I2CD30}: Original DMA Transfer Length Setting and Final Status

New Register mode
{I2CC00}: Master/Slave Function Control Register
{I2CC04}: Master/Slave Clock and AC Timing Control Register
{I2CC08}: Master/Slave Transmit/Receive Byte Buffer Register
{I2CC0C}: Master/Slave Pool Buffer Control Register
{I2CM10}: Master Interrupt Control Register
{I2CM14}: Master Interrupt Status Register  
{I2CM18}: Master Command/Status Register
{I2CM1C}: Master DMA Buffer Length Register
{I2CS20}: Slave~ Interrupt Control Register
{I2CS24}: Slave~ Interrupt Status Register
{I2CS28}: Slave~ Command/Status Register
{I2CS2C}: Slave~ DMA Buffer Length Register
{I2CM30}: Master DMA Mode Tx Buffer Base Address
{I2CM34}: Master DMA Mode Rx Buffer Base Address
{I2CS38}: Slave~ DMA Mode Tx Buffer Base Address
{I2CS3C}: Slave~ DMA Mode Rx Buffer Base Address
{I2CS40}: Slave Device Address Register
{I2CM48}: Master DMA Length Status Register
{I2CS4C}: Slave  DMA Length Status Register
{I2CC50}: Current DMA Operating Address Status
{I2CC54}: Current DMA Operating Length  Status

aspeed,global-regs:
This global register is needed, global register is setting for 
new clock divide control, and new register set control.

ASPEED SOC chip is server product, i2c bus may have
fingerprint connect to another board. And also support hotplug.
The following is board-specific design example.
Board A                                         Board B
-------------------------                       ------------------------
|i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
|i2c bus#2(master)-> tmp i2c device |          |                       |
|i2c bus#3(master)-> adc i2c device |          |                       |
-------------------------                       ------------------------

aspeed,timout properites:
For example I2C controller as slave mode, and suddenly disconnected.
Slave state machine will keep waiting for master clock in for rx/tx transmit.
So it need timeout setting to enable timeout unlock controller state.
And in another side. In Master side also need avoid suddenly slave miss(un-plug),
Master will timeout and release the SDA/SCL.

aspeed,xfer-mode:
For example The bus#1 have trunk data needed for tranfer, it can enable
bus dma mode transfer, it can reduce cpu utilized. bus#2 is small
transmit, it can enable buffer mode or byte mode to reduce memory
cache flush overhead.

v6:
-remove aspeed,i2cv2.yaml, merge to aspeed,i2c.yaml
 -add support for i2cv2 properites.
-i2c-ast2600.c
 -fix ast2600_i2c_remove ordering.
 -remove ast2600_i2c_probe goto labels, and add dev_err_probe
 -remove redundant deb_dbg debug message.
 -rename gr_regmap -> global_regs

v5:
-remove ast2600-i2c-global.yaml, i2c-ast2600-global.c.
-i2c-ast2600.c
 -remove legacy clock divide, all go for new clock divide.
 -remove duplicated read isr.
 -remove no used driver match
 -fix probe return for each labels return.
 -global use mfd driver, driver use phandle to regmap read/write.
-rename aspeed,i2c-ast2600.yaml to aspeed,i2cv2.yaml
-remove bus-frequency.
-add required aspeed,gr
-add timeout, byte-mode, buff-mode properites.

v4:
-fix i2c-ast2600.c driver buffer mode use single buffer conflit in
 master slave mode both enable.
-fix kmemleak issue when use dma mode.
-fix typo aspeed,i2c-ast2600.yaml compatible is "aspeed,ast2600-i2c"
-fix typo aspeed,i2c-ast2600.ymal to aspeed,i2c-ast2600.yaml

v3:
-fix i2c global clock divide default value.
-remove i2c slave no used dev_dbg info.

v2:
-add i2c global ymal file commit.
-rename file name from new to ast2600.
 aspeed-i2c-new-global.c -> i2c-ast2600-global.c
 aspeed-i2c-new-global.h -> i2c-ast2600-global.h
 i2c-new-aspeed.c -> i2c-ast2600.c
-rename all driver function name to ast2600.

Ryan Chen (2):
  dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  i2c: aspeed: support ast2600 i2cv new register mode driver

 .../devicetree/bindings/i2c/aspeed,i2c.yaml   |   44 +
 MAINTAINERS                                   |    9 +
 drivers/i2c/busses/Kconfig                    |   11 +
 drivers/i2c/busses/Makefile                   |    1 +
 drivers/i2c/busses/i2c-ast2600.c              | 1630 +++++++++++++++++
 5 files changed, 1695 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ast2600.c

-- 
2.34.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-02-26  3:13 [PATCH v6 0/2] Add ASPEED AST2600 I2Cv2 controller driver Ryan Chen
@ 2023-02-26  3:13 ` Ryan Chen
  2023-02-26  7:04   ` Jeremy Kerr
                     ` (2 more replies)
  2023-02-26  3:13 ` [PATCH v6 2/2] i2c: aspeed: support ast2600 i2cv new register mode driver Ryan Chen
  1 sibling, 3 replies; 28+ messages in thread
From: Ryan Chen @ 2023-02-26  3:13 UTC (permalink / raw)
  To: Ryan Chen, Andrew Jeffery, Brendan Higgins,
	Benjamin Herrenschmidt, Joel Stanley, Rob Herring,
	Krzysztof Kozlowski, Philipp Zabel, linux-i2c, openbmc,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel

Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout
aspeed,xfer-mode description for ast2600-i2cv2.

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
index f597f73ccd87..75de3ce41cf5 100644
--- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
@@ -49,6 +49,25 @@ properties:
     description:
       states that there is another master active on this bus
 
+  aspeed,timeout:
+    type: boolean
+    description: I2C bus timeout enable for master/slave mode
+
+  aspeed,xfer-mode:
+    description: |
+      I2C bus transfer mode selection.
+      - "byte": I2C bus byte transfer mode.
+      - "buffered": I2C bus buffer register transfer mode.
+      - "dma": I2C bus dma transfer mode (default)
+    items:
+      enum: [byte, buffered, dma]
+    maxItems: 1
+    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
+
+  aspeed,global-regs:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: The phandle of i2c global register node.
+
 required:
   - reg
   - compatible
@@ -57,6 +76,19 @@ required:
 
 unevaluatedProperties: false
 
+if:
+  properties:
+    compatible:
+      contains:
+        const: aspeed,ast2600-i2cv2
+
+then:
+  properties:
+    reg:
+      minItems: 2
+  required:
+    - aspeed,global-regs
+
 examples:
   - |
     #include <dt-bindings/clock/aspeed-clock.h>
@@ -71,3 +103,15 @@ examples:
       interrupts = <0>;
       interrupt-parent = <&i2c_ic>;
     };
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    i2c1: i2c@80 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      compatible = "aspeed,ast2600-i2cv2";
+      reg = <0x80 0x80>, <0xc00 0x20>;
+      interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
+      aspeed,global-regs = <&i2c_global>;
+      clocks = <&syscon ASPEED_CLK_APB>;
+      resets = <&syscon ASPEED_RESET_I2C>;
+    };
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 2/2] i2c: aspeed: support ast2600 i2cv new register mode driver
  2023-02-26  3:13 [PATCH v6 0/2] Add ASPEED AST2600 I2Cv2 controller driver Ryan Chen
  2023-02-26  3:13 ` [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2 Ryan Chen
@ 2023-02-26  3:13 ` Ryan Chen
  1 sibling, 0 replies; 28+ messages in thread
From: Ryan Chen @ 2023-02-26  3:13 UTC (permalink / raw)
  To: Ryan Chen, Andrew Jeffery, Brendan Higgins,
	Benjamin Herrenschmidt, Joel Stanley, Rob Herring,
	Krzysztof Kozlowski, Philipp Zabel, linux-i2c, openbmc,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel

Add i2c new register mode driver to support AST2600 i2c
new register mode. AST2600 i2c controller have legacy and
new register mode. The new register mode have global register
support 4 base clock for scl clock selection, and new clock
divider mode. The i2c new register mode have separate register
set to control i2c master and slave.

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 MAINTAINERS                      |    9 +
 drivers/i2c/busses/Kconfig       |   11 +
 drivers/i2c/busses/Makefile      |    1 +
 drivers/i2c/busses/i2c-ast2600.c | 1630 ++++++++++++++++++++++++++++++
 4 files changed, 1651 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ast2600.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 749710e22b4d..433ec2309441 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1924,6 +1924,15 @@ F:	Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-i2c-ic.
 F:	drivers/i2c/busses/i2c-aspeed.c
 F:	drivers/irqchip/irq-aspeed-i2c-ic.c
 
+ARM/ASPEED I2CV2 DRIVER
+M:	Ryan Chen <ryan_chen@aspeedtech.com>
+R:	Andrew Jeffery <andrew@aj.id.au>
+L:	linux-i2c@vger.kernel.org
+L:	openbmc@lists.ozlabs.org (moderated for non-subscribers)
+S:	Maintained
+F:	Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
+F:	drivers/i2c/busses/i2c-ast2600.c
+
 ARM/ASPEED MACHINE SUPPORT
 M:	Joel Stanley <joel@jms.id.au>
 R:	Andrew Jeffery <andrew@aj.id.au>
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 7284206b278b..4f6935866257 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -389,6 +389,17 @@ config I2C_ALTERA
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-altera.
 
+config I2C_AST2600
+	tristate "Aspeed I2C v2 Controller"
+	depends on ARCH_ASPEED || COMPILE_TEST
+	select I2C_SMBUS
+	help
+	  If you say yes to this option, support will be included for the
+	  Aspeed I2C controller with new register set.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-ast2600.
+
 config I2C_ASPEED
 	tristate "Aspeed I2C Controller"
 	depends on ARCH_ASPEED || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index c5cac15f075c..d0ab4d45781c 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_I2C_POWERMAC)	+= i2c-powermac.o
 obj-$(CONFIG_I2C_ALTERA)	+= i2c-altera.o
 obj-$(CONFIG_I2C_AMD_MP2)	+= i2c-amd-mp2-pci.o i2c-amd-mp2-plat.o
 obj-$(CONFIG_I2C_ASPEED)	+= i2c-aspeed.o
+obj-$(CONFIG_I2C_AST2600)	+= i2c-ast2600.o
 obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
 i2c-at91-objs			:= i2c-at91-core.o i2c-at91-master.o
 ifeq ($(CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL),y)
diff --git a/drivers/i2c/busses/i2c-ast2600.c b/drivers/i2c/busses/i2c-ast2600.c
new file mode 100644
index 000000000000..419e9050481a
--- /dev/null
+++ b/drivers/i2c/busses/i2c-ast2600.c
@@ -0,0 +1,1630 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ASPEED AST2600 new register set I2C controller driver
+ *
+ * Copyright (C) ASPEED Technology Inc.
+ */
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/reset.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/completion.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/of_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/i2c-smbus.h>
+
+/*
+ * APB clk : 100Mhz
+ * div	: scl		: baseclk [APB/((div/2) + 1)] : tBuf [1/bclk * 16]
+ * I2CG10[31:24] base clk4 for i2c auto recovery timeout counter (0xC6)
+ * I2CG10[23:16] base clk3 for Standard-mode (100Khz) min tBuf 4.7us
+ * 0x3c : 100.8Khz	: 3.225Mhz					  : 4.96us
+ * 0x3d : 99.2Khz	: 3.174Mhz					  : 5.04us
+ * 0x3e : 97.65Khz	: 3.125Mhz					  : 5.12us
+ * 0x40 : 97.75Khz	: 3.03Mhz					  : 5.28us
+ * 0x41 : 99.5Khz	: 2.98Mhz					  : 5.36us (default)
+ * I2CG10[15:8] base clk2 for Fast-mode (400Khz) min tBuf 1.3us
+ * 0x12 : 400Khz	: 10Mhz						  : 1.6us
+ * I2CG10[7:0] base clk1 for Fast-mode Plus (1Mhz) min tBuf 0.5us
+ * 0x08 : 1Mhz		: 20Mhz						  : 0.8us
+ */
+#define AST2600_I2CG_ISR			0x00
+#define AST2600_I2CG_SLAVE_ISR		0x04
+#define AST2600_I2CG_OWNER		0x08
+#define AST2600_I2CG_CTRL		0x0C
+#define AST2600_I2CG_CLK_DIV_CTRL	0x10
+
+#define AST2600_I2CG_SLAVE_PKT_NAK	BIT(4)
+#define AST2600_I2CG_M_S_SEPARATE_INTR	BIT(3)
+#define AST2600_I2CG_CTRL_NEW_REG	BIT(2)
+#define AST2600_I2CG_CTRL_NEW_CLK_DIV	BIT(1)
+
+#define AST2600_GLOBAL_INIT				\
+			(AST2600_I2CG_CTRL_NEW_REG |	\
+			AST2600_I2CG_CTRL_NEW_CLK_DIV)
+#define I2CCG_DIV_CTRL 0xC6411208
+
+/* 0x00 : I2CC Master/Slave Function Control Register  */
+#define AST2600_I2CC_FUN_CTRL		0x00
+#define AST2600_I2CC_SLAVE_ADDR_RX_EN		BIT(20)
+#define AST2600_I2CC_MASTER_RETRY_MASK		GENMASK(19, 18)
+#define AST2600_I2CC_MASTER_RETRY(x)		(((x) & GENMASK(1, 0)) << 18)
+#define AST2600_I2CC_BUS_AUTO_RELEASE		BIT(17)
+#define AST2600_I2CC_M_SDA_LOCK_EN			BIT(16)
+#define AST2600_I2CC_MULTI_MASTER_DIS		BIT(15)
+#define AST2600_I2CC_M_SCL_DRIVE_EN			BIT(14)
+#define AST2600_I2CC_MSB_STS				BIT(9)
+#define AST2600_I2CC_SDA_DRIVE_1T_EN		BIT(8)
+#define AST2600_I2CC_M_SDA_DRIVE_1T_EN		BIT(7)
+#define AST2600_I2CC_M_HIGH_SPEED_EN		BIT(6)
+/* reserver 5 : 2 */
+#define AST2600_I2CC_SLAVE_EN			BIT(1)
+#define AST2600_I2CC_MASTER_EN			BIT(0)
+
+/* 0x04 : I2CC Master/Slave Clock and AC Timing Control Register #1 */
+#define AST2600_I2CC_AC_TIMING		0x04
+#define AST2600_I2CC_TTIMEOUT(x)			(((x) & GENMASK(4, 0)) << 24)
+#define AST2600_I2CC_TCKHIGHMIN(x)			(((x) & GENMASK(3, 0)) << 20)
+#define AST2600_I2CC_TCKHIGH(x)			(((x) & GENMASK(3, 0)) << 16)
+#define AST2600_I2CC_TCKLOW(x)			(((x) & GENMASK(3, 0)) << 12)
+#define AST2600_I2CC_THDDAT(x)			(((x) & GENMASK(1, 0)) << 10)
+#define AST2600_I2CC_TOUTBASECLK(x)			(((x) & GENMASK(1, 0)) << 8)
+#define AST2600_I2CC_TBASECLK(x)			((x) & GENMASK(3, 0))
+
+/* 0x08 : I2CC Master/Slave Transmit/Receive Byte Buffer Register */
+#define AST2600_I2CC_STS_AND_BUFF		0x08
+#define AST2600_I2CC_TX_DIR_MASK			GENMASK(31, 29)
+#define AST2600_I2CC_SDA_OE				BIT(28)
+#define AST2600_I2CC_SDA_O				BIT(27)
+#define AST2600_I2CC_SCL_OE				BIT(26)
+#define AST2600_I2CC_SCL_O				BIT(25)
+
+#define AST2600_I2CC_SCL_LINE_STS			BIT(18)
+#define AST2600_I2CC_SDA_LINE_STS			BIT(17)
+#define AST2600_I2CC_BUS_BUSY_STS			BIT(16)
+
+#define AST2600_I2CC_GET_RX_BUFF(x)			(((x) >> 8) & GENMASK(7, 0))
+
+/* 0x0C : I2CC Master/Slave Pool Buffer Control Register  */
+#define AST2600_I2CC_BUFF_CTRL		0x0C
+#define AST2600_I2CC_GET_RX_BUF_LEN(x)		(((x) >> 24) & GENMASK(5, 0))
+#define AST2600_I2CC_SET_RX_BUF_LEN(x)		(((((x) - 1) & GENMASK(4, 0)) << 16) | BIT(0))
+#define AST2600_I2CC_SET_TX_BUF_LEN(x)		(((((x) - 1) & GENMASK(4, 0)) << 8) | BIT(0))
+#define AST2600_I2CC_GET_TX_BUF_LEN(x)		((((x) >> 8) & GENMASK(4, 0)) + 1)
+
+/* 0x10 : I2CM Master Interrupt Control Register */
+#define AST2600_I2CM_IER			0x10
+/* 0x14 : I2CM Master Interrupt Status Register   : WC */
+#define AST2600_I2CM_ISR			0x14
+
+#define AST2600_I2CM_PKT_TIMEOUT			BIT(18)
+#define AST2600_I2CM_PKT_ERROR			BIT(17)
+#define AST2600_I2CM_PKT_DONE			BIT(16)
+
+#define AST2600_I2CM_BUS_RECOVER_FAIL		BIT(15)
+#define AST2600_I2CM_SDA_DL_TO			BIT(14)
+#define AST2600_I2CM_BUS_RECOVER			BIT(13)
+#define AST2600_I2CM_SMBUS_ALT			BIT(12)
+
+#define AST2600_I2CM_SCL_LOW_TO			BIT(6)
+#define AST2600_I2CM_ABNORMAL			BIT(5)
+#define AST2600_I2CM_NORMAL_STOP			BIT(4)
+#define AST2600_I2CM_ARBIT_LOSS			BIT(3)
+#define AST2600_I2CM_RX_DONE			BIT(2)
+#define AST2600_I2CM_TX_NAK				BIT(1)
+#define AST2600_I2CM_TX_ACK				BIT(0)
+
+/* 0x18 : I2CM Master Command/Status Register   */
+#define AST2600_I2CM_CMD_STS		0x18
+#define AST2600_I2CM_PKT_ADDR(x)			(((x) & GENMASK(6, 0)) << 24)
+#define AST2600_I2CM_PKT_EN				BIT(16)
+#define AST2600_I2CM_SDA_OE_OUT_DIR			BIT(15)
+#define AST2600_I2CM_SDA_O_OUT_DIR			BIT(14)
+#define AST2600_I2CM_SCL_OE_OUT_DIR			BIT(13)
+#define AST2600_I2CM_SCL_O_OUT_DIR			BIT(12)
+#define AST2600_I2CM_RECOVER_CMD_EN			BIT(11)
+
+#define AST2600_I2CM_RX_DMA_EN			BIT(9)
+#define AST2600_I2CM_TX_DMA_EN			BIT(8)
+/* Command Bit */
+#define AST2600_I2CM_RX_BUFF_EN			BIT(7)
+#define AST2600_I2CM_TX_BUFF_EN			BIT(6)
+#define AST2600_I2CM_STOP_CMD			BIT(5)
+#define AST2600_I2CM_RX_CMD_LAST			BIT(4)
+#define AST2600_I2CM_RX_CMD				BIT(3)
+
+#define AST2600_I2CM_TX_CMD				BIT(1)
+#define AST2600_I2CM_START_CMD			BIT(0)
+
+/* 0x1C : I2CM Master DMA Transfer Length Register	 */
+#define AST2600_I2CM_DMA_LEN		0x1C
+/* Tx Rx support length 1 ~ 4096 */
+#define AST2600_I2CM_SET_RX_DMA_LEN(x)	((((x) & GENMASK(11, 0)) << 16) | BIT(31))
+#define AST2600_I2CM_SET_TX_DMA_LEN(x)	(((x) & GENMASK(11, 0)) | BIT(15))
+
+/* 0x20 : I2CS Slave Interrupt Control Register   */
+#define AST2600_I2CS_IER			0x20
+/* 0x24 : I2CS Slave Interrupt Status Register	 */
+#define AST2600_I2CS_ISR			0x24
+
+#define AST2600_I2CS_ADDR_INDICATE_MASK	GENMASK(31, 30)
+#define AST2600_I2CS_SLAVE_PENDING			BIT(29)
+
+#define AST2600_I2CS_WAIT_TX_DMA			BIT(25)
+#define AST2600_I2CS_WAIT_RX_DMA			BIT(24)
+
+#define AST2600_I2CS_ADDR3_NAK			BIT(22)
+#define AST2600_I2CS_ADDR2_NAK			BIT(21)
+#define AST2600_I2CS_ADDR1_NAK			BIT(20)
+
+#define AST2600_I2CS_ADDR_MASK			GENMASK(19, 18)
+#define AST2600_I2CS_PKT_ERROR			BIT(17)
+#define AST2600_I2CS_PKT_DONE			BIT(16)
+#define AST2600_I2CS_INACTIVE_TO			BIT(15)
+
+#define AST2600_I2CS_SLAVE_MATCH			BIT(7)
+#define AST2600_I2CS_ABNOR_STOP			BIT(5)
+#define AST2600_I2CS_STOP				BIT(4)
+#define AST2600_I2CS_RX_DONE_NAK			BIT(3)
+#define AST2600_I2CS_RX_DONE			BIT(2)
+#define AST2600_I2CS_TX_NAK				BIT(1)
+#define AST2600_I2CS_TX_ACK				BIT(0)
+
+/* 0x28 : I2CS Slave CMD/Status Register   */
+#define AST2600_I2CS_CMD_STS		0x28
+#define AST2600_I2CS_ACTIVE_ALL			GENMASK(18, 17)
+#define AST2600_I2CS_PKT_MODE_EN			BIT(16)
+#define AST2600_I2CS_AUTO_NAK_NOADDR		BIT(15)
+#define AST2600_I2CS_AUTO_NAK_EN			BIT(14)
+
+#define AST2600_I2CS_ALT_EN				BIT(10)
+#define AST2600_I2CS_RX_DMA_EN			BIT(9)
+#define AST2600_I2CS_TX_DMA_EN			BIT(8)
+#define AST2600_I2CS_RX_BUFF_EN			BIT(7)
+#define AST2600_I2CS_TX_BUFF_EN			BIT(6)
+#define AST2600_I2CS_RX_CMD_LAST			BIT(4)
+
+#define AST2600_I2CS_TX_CMD				BIT(2)
+
+#define AST2600_I2CS_DMA_LEN		0x2C
+#define AST2600_I2CS_SET_RX_DMA_LEN(x)	(((((x) - 1) & GENMASK(11, 0)) << 16) | BIT(31))
+#define AST2600_I2CS_RX_DMA_LEN_MASK	(GENMASK(11, 0) << 16)
+
+#define AST2600_I2CS_SET_TX_DMA_LEN(x)	((((x) - 1) & GENMASK(11, 0)) | BIT(15))
+#define AST2600_I2CS_TX_DMA_LEN_MASK	GENMASK(11, 0)
+
+/* I2CM Master DMA Tx Buffer Register   */
+#define AST2600_I2CM_TX_DMA			0x30
+/* I2CM Master DMA Rx Buffer Register	*/
+#define AST2600_I2CM_RX_DMA			0x34
+/* I2CS Slave DMA Tx Buffer Register   */
+#define AST2600_I2CS_TX_DMA			0x38
+/* I2CS Slave DMA Rx Buffer Register   */
+#define AST2600_I2CS_RX_DMA			0x3C
+
+#define AST2600_I2CS_ADDR_CTRL		0x40
+
+#define	AST2600_I2CS_ADDR3_MASK		GENMASK(22, 16)
+#define	AST2600_I2CS_ADDR2_MASK		GENMASK(14, 8)
+#define	AST2600_I2CS_ADDR1_MASK		GENMASK(6, 0)
+
+#define AST2600_I2CM_DMA_LEN_STS		0x48
+#define AST2600_I2CS_DMA_LEN_STS		0x4C
+
+#define AST2600_I2C_GET_TX_DMA_LEN(x)		((x) & GENMASK(12, 0))
+#define AST2600_I2C_GET_RX_DMA_LEN(x)		(((x) >> 16) & GENMASK(12, 0))
+
+/* 0x40 : Slave Device Address Register */
+#define AST2600_I2CS_ADDR3_ENABLE			BIT(23)
+#define AST2600_I2CS_ADDR3(x)			((x) << 16)
+#define AST2600_I2CS_ADDR2_ENABLE			BIT(15)
+#define AST2600_I2CS_ADDR2(x)			((x) << 8)
+#define AST2600_I2CS_ADDR1_ENABLE			BIT(7)
+#define AST2600_I2CS_ADDR1(x)			(x)
+
+#define I2C_SLAVE_MSG_BUF_SIZE		256
+
+#define AST2600_I2C_DMA_SIZE		4096
+
+#define MASTER_TRIGGER_LAST_STOP	(AST2600_I2CM_RX_CMD_LAST | AST2600_I2CM_STOP_CMD)
+#define SLAVE_TRIGGER_CMD	(AST2600_I2CS_ACTIVE_ALL | AST2600_I2CS_PKT_MODE_EN)
+
+/* i2c timeout counter: use base clk4 1Mhz
+ * unit: 1/(1000/4096) = 4.096ms * 8 = 32.768ms
+ */
+#define AST_I2C_TIMEOUT_CLK		0x2
+#define AST_I2C_TIMEOUT_COUNT	0x8
+
+struct ast2600_i2c_timing_table {
+	u32 divisor;
+	u32 timing;
+};
+
+enum xfer_mode {
+	BYTE_MODE = 0,
+	BUFF_MODE,
+	DMA_MODE,
+};
+
+struct ast2600_xfer_mode_str2field {
+	const char	*name;
+	enum xfer_mode	mode;
+};
+
+static const struct ast2600_xfer_mode_str2field ast2600_xfer_mode_type[] = {
+	{ "byte", BYTE_MODE },
+	{ "buffered", BUFF_MODE },
+	{ "dma", DMA_MODE },
+	{},
+};
+
+struct ast2600_i2c_bus {
+	struct i2c_adapter		adap;
+	struct device			*dev;
+	void __iomem			*reg_base;
+	struct regmap			*global_regs;
+	struct reset_control		*rst;
+	int				irq;
+	enum xfer_mode			mode;
+	struct clk			*clk;
+	u32				apb_clk;
+	u32				bus_frequency;
+	int				slave_operate;
+	int				timeout_enable;
+	/* smbus alert */
+	int				alert_enable;
+	struct i2c_smbus_alert_setup	alert_data;
+	struct i2c_client		*ara;
+	/* Multi-master */
+	bool				multi_master;
+	/* master structure */
+	int				cmd_err;
+	struct completion		cmd_complete;
+	struct i2c_msg			*msgs;
+	size_t				buf_index;
+	/* cur xfer msgs index*/
+	int				msgs_index;
+	int				msgs_count;
+	u8				*master_safe_buf;
+	dma_addr_t			master_dma_addr;
+	/*total xfer count */
+	int				master_xfer_cnt;
+	/* Buffer mode */
+	void __iomem			*buf_base;
+	size_t				buf_size;
+	/* Slave structure */
+	int				slave_xfer_len;
+	int				slave_xfer_cnt;
+#ifdef CONFIG_I2C_SLAVE
+	unsigned char			*slave_dma_buf;
+	dma_addr_t			slave_dma_addr;
+	struct i2c_client		*slave;
+#endif
+};
+
+static u32 ast2600_select_i2c_clock(struct ast2600_i2c_bus *i2c_bus)
+{
+	unsigned long base_clk1;
+	unsigned long base_clk2;
+	unsigned long base_clk3;
+	unsigned long base_clk4;
+	int baseclk_idx;
+	u32 clk_div_reg;
+	u32 scl_low;
+	u32 scl_high;
+	int divisor;
+	int inc = 0;
+	u32 data;
+
+	regmap_read(i2c_bus->global_regs, AST2600_I2CG_CLK_DIV_CTRL, &clk_div_reg);
+	base_clk1 = (i2c_bus->apb_clk * 10) / ((((clk_div_reg & 0xff) + 2) * 10) / 2);
+	base_clk2 = (i2c_bus->apb_clk * 10) /
+			(((((clk_div_reg >> 8) & 0xff) + 2) * 10) / 2);
+	base_clk3 = (i2c_bus->apb_clk * 10) /
+			(((((clk_div_reg >> 16) & 0xff) + 2) * 10) / 2);
+	base_clk4 = (i2c_bus->apb_clk * 10) /
+			(((((clk_div_reg >> 24) & 0xff) + 2) * 10) / 2);
+
+	if ((i2c_bus->apb_clk / i2c_bus->bus_frequency) <= 32) {
+		baseclk_idx = 0;
+		divisor = DIV_ROUND_UP(i2c_bus->apb_clk, i2c_bus->bus_frequency);
+	} else if ((base_clk1 / i2c_bus->bus_frequency) <= 32) {
+		baseclk_idx = 1;
+		divisor = DIV_ROUND_UP(base_clk1, i2c_bus->bus_frequency);
+	} else if ((base_clk2 / i2c_bus->bus_frequency) <= 32) {
+		baseclk_idx = 2;
+		divisor = DIV_ROUND_UP(base_clk2, i2c_bus->bus_frequency);
+	} else if ((base_clk3 / i2c_bus->bus_frequency) <= 32) {
+		baseclk_idx = 3;
+		divisor = DIV_ROUND_UP(base_clk3, i2c_bus->bus_frequency);
+	} else {
+		baseclk_idx = 4;
+		divisor = DIV_ROUND_UP(base_clk4, i2c_bus->bus_frequency);
+		inc = 0;
+		while ((divisor + inc) > 32) {
+			inc |= divisor & 0x1;
+			divisor >>= 1;
+			baseclk_idx++;
+		}
+		divisor += inc;
+	}
+	divisor = min_t(int, divisor, 32);
+	baseclk_idx &= 0xf;
+	scl_low = ((divisor * 9) / 16) - 1;
+	scl_low = min_t(u32, scl_low, 0xf);
+	scl_high = (divisor - scl_low - 2) & 0xf;
+	/* Divisor : Base Clock : tCKHighMin : tCK High : tCK Low  */
+	data = ((scl_high - 1) << 20) | (scl_high << 16) | (scl_low << 12) | (baseclk_idx);
+	if (i2c_bus->timeout_enable) {
+		data |= AST2600_I2CC_TOUTBASECLK(AST_I2C_TIMEOUT_CLK);
+		data |= AST2600_I2CC_TTIMEOUT(AST_I2C_TIMEOUT_COUNT);
+	}
+
+	return data;
+}
+
+static u8 ast2600_i2c_recover_bus(struct ast2600_i2c_bus *i2c_bus)
+{
+	int ret = 0;
+	u32 ctrl;
+	u32 state;
+	int r;
+
+	dev_dbg(i2c_bus->dev, "%d-bus recovery bus [%x]\n", i2c_bus->adap.nr,
+		readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));
+
+	ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+	writel(ctrl & ~(AST2600_I2CC_MASTER_EN | AST2600_I2CC_SLAVE_EN),
+	       i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+	writel(readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL) | AST2600_I2CC_MASTER_EN,
+	       i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+	reinit_completion(&i2c_bus->cmd_complete);
+	i2c_bus->cmd_err = 0;
+
+	/* Check 0x14's SDA and SCL status */
+	state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
+	if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & AST2600_I2CC_SCL_LINE_STS)) {
+		writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
+		r = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout);
+		if (r == 0) {
+			dev_dbg(i2c_bus->dev, "recovery timed out\n");
+			ret = -ETIMEDOUT;
+		} else {
+			if (i2c_bus->cmd_err) {
+				dev_dbg(i2c_bus->dev, "recovery error\n");
+				ret = -EPROTO;
+			}
+		}
+	}
+
+	dev_dbg(i2c_bus->dev, "Recovery done [%x]\n",
+		readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));
+	if (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) & AST2600_I2CC_BUS_BUSY_STS) {
+		dev_dbg(i2c_bus->dev, "Can't recovery bus [%x]\n",
+			readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));
+		ret = -EPROTO;
+	}
+
+	writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+	return ret;
+}
+
+#ifdef CONFIG_I2C_SLAVE
+static void ast2600_i2c_slave_packet_dma_irq(struct ast2600_i2c_bus *i2c_bus, u32 sts)
+{
+	int slave_rx_len;
+	u32 cmd = 0;
+	u8 value;
+	int i = 0;
+
+	sts &= ~(AST2600_I2CS_SLAVE_PENDING);
+	/* Handle i2c slave timeout condition */
+	if (AST2600_I2CS_INACTIVE_TO & sts) {
+		cmd = SLAVE_TRIGGER_CMD;
+		cmd |= AST2600_I2CS_RX_DMA_EN;
+		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
+		       i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
+		writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+		writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_ISR);
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
+		return;
+	}
+
+	sts &= ~(AST2600_I2CS_PKT_DONE | AST2600_I2CS_PKT_ERROR);
+
+	switch (sts) {
+	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_RX_DMA:
+	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_WAIT_RX_DMA:
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, &value);
+		slave_rx_len = AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
+						      AST2600_I2CS_DMA_LEN_STS));
+		for (i = 0; i < slave_rx_len; i++) {
+			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED,
+					&i2c_bus->slave_dma_buf[i]);
+		}
+		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
+				i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
+		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
+		break;
+	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_STOP:
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
+		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
+				i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
+		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
+		break;
+	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE_NAK |
+			AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
+	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_WAIT_RX_DMA |
+			AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
+	case AST2600_I2CS_RX_DONE_NAK | AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
+	case AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_STOP:
+	case AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
+	case AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_RX_DMA:
+	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
+		if (sts & AST2600_I2CS_SLAVE_MATCH)
+			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, &value);
+
+		slave_rx_len = AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
+						      AST2600_I2CS_DMA_LEN_STS));
+		for (i = 0; i < slave_rx_len; i++) {
+			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED,
+					&i2c_bus->slave_dma_buf[i]);
+		}
+		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
+		       i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
+		if (sts & AST2600_I2CS_STOP)
+			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
+		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
+		break;
+
+	/* it is Mw data Mr coming -> it need send tx */
+	case AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_TX_DMA:
+	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_TX_DMA:
+		/* it should be repeat start read */
+		if (sts & AST2600_I2CS_SLAVE_MATCH)
+			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, &value);
+
+		slave_rx_len = AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
+						      AST2600_I2CS_DMA_LEN_STS));
+		for (i = 0; i < slave_rx_len; i++) {
+			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED,
+					&i2c_bus->slave_dma_buf[i]);
+		}
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED,
+				&i2c_bus->slave_dma_buf[0]);
+		writel(0, i2c_bus->reg_base + AST2600_I2CS_DMA_LEN_STS);
+		writel(AST2600_I2CS_SET_TX_DMA_LEN(1),
+				i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
+		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_DMA_EN;
+		break;
+	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_WAIT_TX_DMA:
+		/* First Start read */
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED,
+				&i2c_bus->slave_dma_buf[0]);
+		writel(AST2600_I2CS_SET_TX_DMA_LEN(1),
+				i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
+		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_DMA_EN;
+		break;
+	case AST2600_I2CS_WAIT_TX_DMA:
+		/* it should be next start read */
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_PROCESSED,
+				&i2c_bus->slave_dma_buf[0]);
+		writel(0, i2c_bus->reg_base + AST2600_I2CS_DMA_LEN_STS);
+		writel(AST2600_I2CS_SET_TX_DMA_LEN(1),
+				i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
+		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_DMA_EN;
+		break;
+
+	case AST2600_I2CS_TX_NAK | AST2600_I2CS_STOP:
+		/* it just tx complete */
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
+		writel(0, i2c_bus->reg_base + AST2600_I2CS_DMA_LEN_STS);
+		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
+		       i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
+		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
+		break;
+	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:
+		cmd = 0;
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, &value);
+		break;
+	case AST2600_I2CS_STOP:
+		cmd = 0;
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
+		break;
+	default:
+		dev_dbg(i2c_bus->dev, "unhandled slave isr case %x, sts %x\n", sts,
+			readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));
+		break;
+	}
+
+	if (cmd)
+		writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+	writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_ISR);
+	readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
+}
+
+static void ast2600_i2c_slave_packet_buff_irq(struct ast2600_i2c_bus *i2c_bus, u32 sts)
+{
+	int slave_rx_len = 0;
+	u32 cmd = 0;
+	u8 value;
+	int i = 0;
+
+	/* due to master slave is common buffer, so need force the master stop not issue */
+	if (readl(i2c_bus->reg_base + AST2600_I2CM_CMD_STS) & 0xffff) {
+		writel(0, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
+		i2c_bus->cmd_err = -EBUSY;
+		writel(0, i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+		complete(&i2c_bus->cmd_complete);
+	}
+
+	/* Handle i2c slave timeout condition */
+	if (AST2600_I2CS_INACTIVE_TO & sts) {
+		writel(SLAVE_TRIGGER_CMD, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+		writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_ISR);
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
+		i2c_bus->slave_operate = 0;
+		return;
+	}
+
+	sts &= ~(AST2600_I2CS_PKT_DONE | AST2600_I2CS_PKT_ERROR);
+
+	if (sts & AST2600_I2CS_SLAVE_MATCH)
+		i2c_bus->slave_operate = 1;
+
+	switch (sts) {
+	case AST2600_I2CS_SLAVE_PENDING | AST2600_I2CS_WAIT_RX_DMA |
+		 AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
+	case AST2600_I2CS_SLAVE_PENDING |
+		 AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
+	case AST2600_I2CS_SLAVE_PENDING |
+		 AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_STOP:
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
+		fallthrough;
+	case AST2600_I2CS_SLAVE_PENDING |
+		 AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:
+	case AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:
+	case AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_SLAVE_MATCH:
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, &value);
+		cmd = SLAVE_TRIGGER_CMD;
+		if (sts & AST2600_I2CS_RX_DONE) {
+			slave_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
+							       AST2600_I2CC_BUFF_CTRL));
+			for (i = 0; i < slave_rx_len; i++) {
+				value = readb(i2c_bus->buf_base + 0x10 + i);
+				i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED, &value);
+			}
+		}
+		if (readl(i2c_bus->reg_base + AST2600_I2CS_CMD_STS) & AST2600_I2CS_RX_BUFF_EN)
+			cmd = 0;
+		else
+			cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_BUFF_EN;
+
+		writel(AST2600_I2CC_SET_RX_BUF_LEN(i2c_bus->buf_size),
+					i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+		break;
+	case AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_RX_DONE:
+		cmd = SLAVE_TRIGGER_CMD;
+		slave_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
+						       AST2600_I2CC_BUFF_CTRL));
+		for (i = 0; i < slave_rx_len; i++) {
+			value = readb(i2c_bus->buf_base + 0x10 + i);
+			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED, &value);
+		}
+		cmd |= AST2600_I2CS_RX_BUFF_EN;
+		writel(AST2600_I2CC_SET_RX_BUF_LEN(i2c_bus->buf_size),
+						i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+		break;
+	case AST2600_I2CS_SLAVE_PENDING | AST2600_I2CS_WAIT_RX_DMA |
+				AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
+		cmd = SLAVE_TRIGGER_CMD;
+		slave_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
+						       AST2600_I2CC_BUFF_CTRL));
+		for (i = 0; i < slave_rx_len; i++) {
+			value = readb(i2c_bus->buf_base + 0x10 + i);
+			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED, &value);
+		}
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
+		cmd |= AST2600_I2CS_RX_BUFF_EN;
+		writel(AST2600_I2CC_SET_RX_BUF_LEN(i2c_bus->buf_size),
+						i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+		break;
+
+	case AST2600_I2CS_SLAVE_PENDING | AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
+		cmd = SLAVE_TRIGGER_CMD;
+		slave_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
+							   AST2600_I2CC_BUFF_CTRL));
+		for (i = 0; i < slave_rx_len; i++) {
+			value = readb(i2c_bus->buf_base + 0x10 + i);
+			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED, &value);
+		}
+		/* workaround for avoid next start with len != 0 */
+		writel(BIT(0), i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
+		break;
+
+	case AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
+		cmd = SLAVE_TRIGGER_CMD;
+		slave_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
+							   AST2600_I2CC_BUFF_CTRL));
+		for (i = 0; i < slave_rx_len; i++) {
+			value = readb(i2c_bus->buf_base + 0x10 + i);
+			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED, &value);
+		}
+		/* workaround for avoid next start with len != 0 */
+		writel(BIT(0), i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
+		break;
+	case AST2600_I2CS_WAIT_TX_DMA | AST2600_I2CS_SLAVE_MATCH:
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
+		writeb(value, i2c_bus->buf_base);
+		writel(AST2600_I2CC_SET_TX_BUF_LEN(1),
+				i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_BUFF_EN;
+		break;
+	case AST2600_I2CS_WAIT_TX_DMA | AST2600_I2CS_RX_DONE:
+	case AST2600_I2CS_WAIT_TX_DMA:
+		if (sts & AST2600_I2CS_RX_DONE) {
+			slave_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
+							AST2600_I2CC_BUFF_CTRL));
+			for (i = 0; i < slave_rx_len; i++) {
+				value = readb(i2c_bus->buf_base + 0x10 + i);
+				i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED, &value);
+			}
+			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
+		} else {
+			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
+		}
+		writeb(value, i2c_bus->buf_base);
+		writel(AST2600_I2CC_SET_TX_BUF_LEN(1),
+				i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_BUFF_EN;
+		break;
+	/* workaround : trigger the cmd twice to fix next state keep 1000000 */
+	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, &value);
+		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_BUFF_EN;
+		writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+		break;
+
+	case AST2600_I2CS_TX_NAK | AST2600_I2CS_STOP:
+	case AST2600_I2CS_STOP:
+		cmd = SLAVE_TRIGGER_CMD;
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
+		break;
+	default:
+		dev_dbg(i2c_bus->dev, "unhandled slave isr case %x, sts %x\n", sts,
+			readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));
+		break;
+	}
+
+	if (cmd)
+		writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+	writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_ISR);
+	readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
+
+	if ((sts & AST2600_I2CS_STOP) && !(sts & AST2600_I2CS_SLAVE_PENDING))
+		i2c_bus->slave_operate = 0;
+
+}
+
+static void ast2600_i2c_slave_byte_irq(struct ast2600_i2c_bus *i2c_bus, u32 sts)
+{
+	u32 i2c_buff = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
+	u32 cmd = AST2600_I2CS_ACTIVE_ALL;
+	u8 byte_data;
+	u8 value;
+
+	switch (sts) {
+	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_RX_DMA:
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, &value);
+		/* first address match is address */
+		byte_data = AST2600_I2CC_GET_RX_BUFF(i2c_buff);
+		break;
+	case AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_RX_DMA:
+		byte_data = AST2600_I2CC_GET_RX_BUFF(i2c_buff);
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED, &byte_data);
+		break;
+	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_TX_DMA:
+		cmd |= AST2600_I2CS_TX_CMD;
+		byte_data = AST2600_I2CC_GET_RX_BUFF(i2c_buff);
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED, &byte_data);
+		writel(byte_data, i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
+		break;
+	case AST2600_I2CS_TX_ACK | AST2600_I2CS_WAIT_TX_DMA:
+		cmd |= AST2600_I2CS_TX_CMD;
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_PROCESSED, &byte_data);
+		writel(byte_data, i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
+		break;
+	case AST2600_I2CS_STOP:
+	case AST2600_I2CS_STOP | AST2600_I2CS_TX_NAK:
+		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
+		break;
+	default:
+		dev_dbg(i2c_bus->dev, "unhandled pkt isr %x\n", sts);
+		break;
+	}
+	writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+	writel(sts, i2c_bus->reg_base + AST2600_I2CS_ISR);
+	readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
+}
+
+static int ast2600_i2c_slave_irq(struct ast2600_i2c_bus *i2c_bus)
+{
+	u32 ier = readl(i2c_bus->reg_base + AST2600_I2CS_IER);
+	u32 isr = readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
+
+	if (!(isr & ier))
+		return 0;
+
+	/* Slave interrupt coming after Master package done
+	 * So need handle master first.
+	 */
+	if (readl(i2c_bus->reg_base + AST2600_I2CM_ISR) & AST2600_I2CM_PKT_DONE)
+		return 0;
+
+	isr &= ~(AST2600_I2CS_ADDR_INDICATE_MASK);
+
+	if (AST2600_I2CS_ADDR1_NAK & isr)
+		isr &= ~AST2600_I2CS_ADDR1_NAK;
+
+	if (AST2600_I2CS_ADDR2_NAK & isr)
+		isr &= ~AST2600_I2CS_ADDR2_NAK;
+
+	if (AST2600_I2CS_ADDR3_NAK & isr)
+		isr &= ~AST2600_I2CS_ADDR3_NAK;
+
+	if (AST2600_I2CS_ADDR_MASK & isr)
+		isr &= ~AST2600_I2CS_ADDR_MASK;
+
+	if (AST2600_I2CS_PKT_DONE & isr) {
+		if (i2c_bus->mode == DMA_MODE)
+			ast2600_i2c_slave_packet_dma_irq(i2c_bus, isr);
+		else
+			ast2600_i2c_slave_packet_buff_irq(i2c_bus, isr);
+	} else
+		ast2600_i2c_slave_byte_irq(i2c_bus, isr);
+
+	return 1;
+}
+#endif
+
+static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus)
+{
+	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
+	int xfer_len = 0;
+	int i = 0;
+	u32 cmd;
+
+	cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(msg->addr) | AST2600_I2CM_START_CMD;
+
+	/* send start */
+	dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n",
+		i2c_bus->msgs_index, msg->flags & I2C_M_RD ? "read" : "write",
+		msg->len, msg->len > 1 ? "s" : "",
+		msg->flags & I2C_M_RD ? "from" : "to", msg->addr);
+
+	i2c_bus->master_xfer_cnt = 0;
+	i2c_bus->buf_index = 0;
+
+	if (msg->flags & I2C_M_RD) {
+		cmd |= AST2600_I2CM_RX_CMD;
+		if (i2c_bus->mode == DMA_MODE) {
+			/* dma mode */
+			cmd |= AST2600_I2CM_RX_DMA_EN;
+
+			if (msg->flags & I2C_M_RECV_LEN) {
+				xfer_len = 1;
+			} else {
+				if (msg->len > AST2600_I2C_DMA_SIZE) {
+					xfer_len = AST2600_I2C_DMA_SIZE;
+				} else {
+					xfer_len = msg->len;
+					if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+						cmd |= MASTER_TRIGGER_LAST_STOP;
+				}
+			}
+			writel(AST2600_I2CM_SET_RX_DMA_LEN(xfer_len - 1),
+			       i2c_bus->reg_base + AST2600_I2CM_DMA_LEN);
+			i2c_bus->master_safe_buf = i2c_get_dma_safe_msg_buf(msg, 1);
+			if (!i2c_bus->master_safe_buf)
+				return -ENOMEM;
+			i2c_bus->master_dma_addr =
+				dma_map_single(i2c_bus->dev, i2c_bus->master_safe_buf,
+								msg->len, DMA_FROM_DEVICE);
+			if (dma_mapping_error(i2c_bus->dev, i2c_bus->master_dma_addr)) {
+				i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, msg, false);
+				i2c_bus->master_safe_buf = NULL;
+				return -ENOMEM;
+			}
+			writel(i2c_bus->master_dma_addr, i2c_bus->reg_base + AST2600_I2CM_RX_DMA);
+		} else if (i2c_bus->mode == BUFF_MODE) {
+			/* buff mode */
+			cmd |= AST2600_I2CM_RX_BUFF_EN;
+			if (msg->flags & I2C_M_RECV_LEN) {
+				dev_dbg(i2c_bus->dev, "smbus read\n");
+				xfer_len = 1;
+			} else {
+				if (msg->len > i2c_bus->buf_size) {
+					xfer_len = i2c_bus->buf_size;
+				} else {
+					xfer_len = msg->len;
+					if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+						cmd |= MASTER_TRIGGER_LAST_STOP;
+				}
+			}
+			writel(AST2600_I2CC_SET_RX_BUF_LEN(xfer_len),
+			       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+		} else {
+			/* byte mode */
+			xfer_len = 1;
+			if (msg->flags & I2C_M_RECV_LEN) {
+				dev_dbg(i2c_bus->dev, "smbus read\n");
+			} else {
+				if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) {
+					if (msg->len == 1)
+						cmd |= MASTER_TRIGGER_LAST_STOP;
+				}
+			}
+		}
+	} else {
+		if (i2c_bus->mode == DMA_MODE) {
+			/* dma mode */
+			if (msg->len > AST2600_I2C_DMA_SIZE) {
+				xfer_len = AST2600_I2C_DMA_SIZE;
+			} else {
+				if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+					cmd |= AST2600_I2CM_STOP_CMD;
+				xfer_len = msg->len;
+			}
+
+			if (xfer_len) {
+				cmd |= AST2600_I2CM_TX_DMA_EN | AST2600_I2CM_TX_CMD;
+				writel(AST2600_I2CM_SET_TX_DMA_LEN(xfer_len - 1),
+				       i2c_bus->reg_base + AST2600_I2CM_DMA_LEN);
+				i2c_bus->master_safe_buf = i2c_get_dma_safe_msg_buf(msg, 1);
+				if (!i2c_bus->master_safe_buf)
+					return -ENOMEM;
+				i2c_bus->master_dma_addr =
+					dma_map_single(i2c_bus->dev, i2c_bus->master_safe_buf,
+								msg->len, DMA_TO_DEVICE);
+				if (dma_mapping_error(i2c_bus->dev, i2c_bus->master_dma_addr)) {
+					i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf,
+											msg, false);
+					i2c_bus->master_safe_buf = NULL;
+					return -ENOMEM;
+				}
+				writel(i2c_bus->master_dma_addr,
+				       i2c_bus->reg_base + AST2600_I2CM_TX_DMA);
+			}
+		} else if (i2c_bus->mode == BUFF_MODE) {
+			u8 wbuf[4];
+			/* buff mode */
+			if (msg->len > i2c_bus->buf_size) {
+				xfer_len = i2c_bus->buf_size;
+			} else {
+				if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+					cmd |= AST2600_I2CM_STOP_CMD;
+				xfer_len = msg->len;
+			}
+			if (xfer_len) {
+				cmd |= AST2600_I2CM_TX_BUFF_EN | AST2600_I2CM_TX_CMD;
+				if (readl(i2c_bus->reg_base + AST2600_I2CS_ISR))
+					return -ENOMEM;
+				writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len),
+				       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+				if (readl(i2c_bus->reg_base + AST2600_I2CS_ISR))
+					return -ENOMEM;
+				for (i = 0; i < xfer_len; i++) {
+					wbuf[i % 4] = msg->buf[i];
+					if (i % 4 == 3)
+						writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3);
+				}
+				if (--i % 4 != 3)
+					writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4));
+			}
+			if (readl(i2c_bus->reg_base + AST2600_I2CS_ISR))
+				return -ENOMEM;
+		} else {
+			/* byte mode */
+			if ((i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) && (msg->len <= 1))
+				cmd |= AST2600_I2CM_STOP_CMD;
+
+			if (msg->len) {
+				cmd |= AST2600_I2CM_TX_CMD;
+				xfer_len = 1;
+				writel(msg->buf[0], i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
+			} else {
+				xfer_len = 0;
+			}
+		}
+	}
+	writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
+	return 0;
+}
+
+static int ast2600_i2c_is_irq_error(u32 irq_status)
+{
+	if (irq_status & AST2600_I2CM_ARBIT_LOSS)
+		return -EAGAIN;
+	if (irq_status & (AST2600_I2CM_SDA_DL_TO | AST2600_I2CM_SCL_LOW_TO))
+		return -EBUSY;
+	if (irq_status & (AST2600_I2CM_ABNORMAL))
+		return -EPROTO;
+
+	return 0;
+}
+
+static void ast2600_i2c_master_package_irq(struct ast2600_i2c_bus *i2c_bus, u32 sts)
+{
+	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
+	u32 cmd = AST2600_I2CM_PKT_EN;
+	int xfer_len;
+	int i;
+
+	sts &= ~AST2600_I2CM_PKT_DONE;
+	writel(AST2600_I2CM_PKT_DONE, i2c_bus->reg_base + AST2600_I2CM_ISR);
+	switch (sts) {
+	case AST2600_I2CM_PKT_ERROR:
+		i2c_bus->cmd_err = -EAGAIN;
+		complete(&i2c_bus->cmd_complete);
+		break;
+	case AST2600_I2CM_PKT_ERROR | AST2600_I2CM_TX_NAK: /* a0 fix for issue */
+		fallthrough;
+	case AST2600_I2CM_PKT_ERROR | AST2600_I2CM_TX_NAK | AST2600_I2CM_NORMAL_STOP:
+		i2c_bus->cmd_err = -ENXIO;
+		complete(&i2c_bus->cmd_complete);
+		break;
+	case AST2600_I2CM_NORMAL_STOP:
+		/* write 0 byte only have stop isr */
+		i2c_bus->msgs_index++;
+		if (i2c_bus->msgs_index < i2c_bus->msgs_count) {
+			if (ast2600_i2c_do_start(i2c_bus)) {
+				i2c_bus->cmd_err = -ENOMEM;
+				complete(&i2c_bus->cmd_complete);
+			}
+		} else {
+			i2c_bus->cmd_err = i2c_bus->msgs_index;
+			complete(&i2c_bus->cmd_complete);
+		}
+		break;
+	case AST2600_I2CM_TX_ACK:
+	case AST2600_I2CM_TX_ACK | AST2600_I2CM_NORMAL_STOP:
+		if (i2c_bus->mode == DMA_MODE)
+			xfer_len = AST2600_I2C_GET_TX_DMA_LEN(readl(i2c_bus->reg_base +
+							  AST2600_I2CM_DMA_LEN_STS));
+		else if (i2c_bus->mode == BUFF_MODE)
+			xfer_len = AST2600_I2CC_GET_TX_BUF_LEN(readl(i2c_bus->reg_base +
+							   AST2600_I2CC_BUFF_CTRL));
+		else
+			xfer_len = 1;
+
+		i2c_bus->master_xfer_cnt += xfer_len;
+
+		if (i2c_bus->master_xfer_cnt == msg->len) {
+			if (i2c_bus->mode == DMA_MODE) {
+				dma_unmap_single(i2c_bus->dev, i2c_bus->master_dma_addr, msg->len,
+						 DMA_TO_DEVICE);
+				i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, msg, true);
+				i2c_bus->master_safe_buf = NULL;
+			}
+			i2c_bus->msgs_index++;
+			if (i2c_bus->msgs_index == i2c_bus->msgs_count) {
+				i2c_bus->cmd_err = i2c_bus->msgs_index;
+				complete(&i2c_bus->cmd_complete);
+			} else {
+				if (ast2600_i2c_do_start(i2c_bus)) {
+					i2c_bus->cmd_err = -ENOMEM;
+					complete(&i2c_bus->cmd_complete);
+				}
+			}
+		} else {
+			/* do next tx */
+			cmd |= AST2600_I2CM_TX_CMD;
+			if (i2c_bus->mode == DMA_MODE) {
+				cmd |= AST2600_I2CM_TX_DMA_EN;
+				xfer_len = msg->len - i2c_bus->master_xfer_cnt;
+				if (xfer_len > AST2600_I2C_DMA_SIZE) {
+					xfer_len = AST2600_I2C_DMA_SIZE;
+				} else {
+					if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+						cmd |= AST2600_I2CM_STOP_CMD;
+				}
+				writel(AST2600_I2CM_SET_TX_DMA_LEN(xfer_len - 1),
+				       i2c_bus->reg_base + AST2600_I2CM_DMA_LEN);
+				writel(i2c_bus->master_dma_addr + i2c_bus->master_xfer_cnt,
+				       i2c_bus->reg_base + AST2600_I2CM_TX_DMA);
+			} else if (i2c_bus->mode == BUFF_MODE) {
+				u8 wbuf[4];
+
+				cmd |= AST2600_I2CM_TX_BUFF_EN;
+				xfer_len = msg->len - i2c_bus->master_xfer_cnt;
+				if (xfer_len > i2c_bus->buf_size) {
+					xfer_len = i2c_bus->buf_size;
+				} else {
+					if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+						cmd |= AST2600_I2CM_STOP_CMD;
+				}
+				for (i = 0; i < xfer_len; i++) {
+					wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i];
+					if (i % 4 == 3)
+						writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3);
+				}
+				if (--i % 4 != 3)
+					writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4));
+				writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len),
+				       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+			} else {
+				/* byte */
+				if ((i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) &&
+				    ((i2c_bus->master_xfer_cnt + 1) == msg->len)) {
+					cmd |= AST2600_I2CM_STOP_CMD;
+				}
+				writel(msg->buf[i2c_bus->master_xfer_cnt],
+				       i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
+			}
+			writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
+		}
+		break;
+	case AST2600_I2CM_RX_DONE:
+#ifdef CONFIG_I2C_SLAVE
+		/* Workaround for master/slave package mode enable rx done stuck issue
+		 * When master go for first read (RX_DONE), slave mode will also effect
+		 * Then controller will send nack, not operate anymore.
+		 */
+		if (readl(i2c_bus->reg_base + AST2600_I2CS_CMD_STS) & AST2600_I2CS_PKT_MODE_EN) {
+			u32 slave_cmd = readl(i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+
+			writel(0, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+			writel(slave_cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+		}
+		fallthrough;
+#endif
+	case AST2600_I2CM_RX_DONE | AST2600_I2CM_NORMAL_STOP:
+		/* do next rx */
+		if (i2c_bus->mode == DMA_MODE) {
+			xfer_len = AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
+							  AST2600_I2CM_DMA_LEN_STS));
+		} else if (i2c_bus->mode == BUFF_MODE) {
+			xfer_len = AST2600_I2CC_GET_RX_BUF_LEN(
+						readl(i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL));
+			for (i = 0; i < xfer_len; i++)
+				msg->buf[i2c_bus->master_xfer_cnt + i] =
+					readb(i2c_bus->buf_base + 0x10 + i);
+		} else {
+			xfer_len = 1;
+			msg->buf[i2c_bus->master_xfer_cnt] =
+				AST2600_I2CC_GET_RX_BUFF(readl(i2c_bus->reg_base +
+						     AST2600_I2CC_STS_AND_BUFF));
+		}
+
+		if (msg->flags & I2C_M_RECV_LEN) {
+			msg->len = msg->buf[0] + ((msg->flags & I2C_CLIENT_PEC) ? 2 : 1);
+			msg->len = min_t(unsigned int, msg->len, I2C_SMBUS_BLOCK_MAX);
+			msg->flags &= ~I2C_M_RECV_LEN;
+		}
+		i2c_bus->master_xfer_cnt += xfer_len;
+
+		if (i2c_bus->master_xfer_cnt == msg->len) {
+			if (i2c_bus->mode == DMA_MODE) {
+				dma_unmap_single(i2c_bus->dev, i2c_bus->master_dma_addr, msg->len,
+						 DMA_FROM_DEVICE);
+				i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, msg, true);
+				i2c_bus->master_safe_buf = NULL;
+			}
+
+			i2c_bus->msgs_index++;
+			if (i2c_bus->msgs_index == i2c_bus->msgs_count) {
+				i2c_bus->cmd_err = i2c_bus->msgs_index;
+				complete(&i2c_bus->cmd_complete);
+			} else {
+				if (ast2600_i2c_do_start(i2c_bus)) {
+					i2c_bus->cmd_err = -ENOMEM;
+					complete(&i2c_bus->cmd_complete);
+				}
+			}
+		} else {
+			/* next rx */
+			cmd |= AST2600_I2CM_RX_CMD;
+			if (i2c_bus->mode == DMA_MODE) {
+				cmd |= AST2600_I2CM_RX_DMA_EN;
+				xfer_len = msg->len - i2c_bus->master_xfer_cnt;
+				if (xfer_len > AST2600_I2C_DMA_SIZE) {
+					xfer_len = AST2600_I2C_DMA_SIZE;
+				} else {
+					if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+						cmd |= MASTER_TRIGGER_LAST_STOP;
+				}
+				writel(AST2600_I2CM_SET_RX_DMA_LEN(xfer_len - 1),
+				       i2c_bus->reg_base + AST2600_I2CM_DMA_LEN);
+				writel(i2c_bus->master_dma_addr + i2c_bus->master_xfer_cnt,
+				       i2c_bus->reg_base + AST2600_I2CM_RX_DMA);
+			} else if (i2c_bus->mode == BUFF_MODE) {
+				cmd |= AST2600_I2CM_RX_BUFF_EN;
+				xfer_len = msg->len - i2c_bus->master_xfer_cnt;
+				if (xfer_len > i2c_bus->buf_size)
+					xfer_len = i2c_bus->buf_size;
+				else {
+					if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+						cmd |= MASTER_TRIGGER_LAST_STOP;
+				}
+				writel(AST2600_I2CC_SET_RX_BUF_LEN(xfer_len),
+				       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+			} else {
+				if ((i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) &&
+				    ((i2c_bus->master_xfer_cnt + 1) == msg->len)) {
+					cmd |= MASTER_TRIGGER_LAST_STOP;
+				}
+			}
+			writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
+		}
+		break;
+	default:
+		dev_dbg(i2c_bus->dev, "unhandled sts %x\n", sts);
+		break;
+	}
+}
+
+static int ast2600_i2c_master_irq(struct ast2600_i2c_bus *i2c_bus)
+{
+	u32 sts = readl(i2c_bus->reg_base + AST2600_I2CM_ISR);
+	u32 ier = readl(i2c_bus->reg_base + AST2600_I2CM_IER);
+	u32 ctrl = 0;
+
+	if (!i2c_bus->alert_enable)
+		sts &= ~AST2600_I2CM_SMBUS_ALT;
+
+	if (AST2600_I2CM_BUS_RECOVER_FAIL & sts) {
+		writel(AST2600_I2CM_BUS_RECOVER_FAIL, i2c_bus->reg_base + AST2600_I2CM_ISR);
+		ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+		writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+		writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+		i2c_bus->cmd_err = -EPROTO;
+		complete(&i2c_bus->cmd_complete);
+		return 1;
+	}
+
+	if (AST2600_I2CM_BUS_RECOVER & sts) {
+		writel(AST2600_I2CM_BUS_RECOVER, i2c_bus->reg_base + AST2600_I2CM_ISR);
+		i2c_bus->cmd_err = 0;
+		complete(&i2c_bus->cmd_complete);
+		return 1;
+	}
+
+	if (AST2600_I2CM_SMBUS_ALT & sts) {
+		if (ier & AST2600_I2CM_SMBUS_ALT) {
+			/* Disable ALT INT */
+			writel(ier & ~AST2600_I2CM_SMBUS_ALT, i2c_bus->reg_base + AST2600_I2CM_IER);
+			i2c_handle_smbus_alert(i2c_bus->ara);
+			writel(AST2600_I2CM_SMBUS_ALT, i2c_bus->reg_base + AST2600_I2CM_ISR);
+			dev_err(i2c_bus->dev,
+				"ast2600_master_alert_recv bus id %d, Disable Alt, Please Imple\n",
+				i2c_bus->adap.nr);
+			return 1;
+		}
+	}
+
+	i2c_bus->cmd_err = ast2600_i2c_is_irq_error(sts);
+	if (i2c_bus->cmd_err) {
+		writel(AST2600_I2CM_PKT_DONE, i2c_bus->reg_base + AST2600_I2CM_ISR);
+		complete(&i2c_bus->cmd_complete);
+		return 1;
+	}
+
+	if (AST2600_I2CM_PKT_DONE & sts) {
+		ast2600_i2c_master_package_irq(i2c_bus, sts);
+		return 1;
+	}
+
+	return 0;
+}
+
+static irqreturn_t ast2600_i2c_bus_irq(int irq, void *dev_id)
+{
+	struct ast2600_i2c_bus *i2c_bus = dev_id;
+
+#ifdef CONFIG_I2C_SLAVE
+	if (readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL) & AST2600_I2CC_SLAVE_EN) {
+		if (ast2600_i2c_slave_irq(i2c_bus))
+			return IRQ_HANDLED;
+	}
+#endif
+	return ast2600_i2c_master_irq(i2c_bus) ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static int ast2600_i2c_master_xfer(struct i2c_adapter *adap,
+				      struct i2c_msg *msgs, int num)
+{
+	struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(adap);
+	unsigned long timeout;
+	int ret = 0;
+
+	/* If bus is busy in a single master environment, attempt recovery. */
+	if (!i2c_bus->multi_master &&
+	    (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) & AST2600_I2CC_BUS_BUSY_STS)) {
+		int ret;
+
+		ret = ast2600_i2c_recover_bus(i2c_bus);
+		if (ret)
+			return ret;
+	}
+
+#ifdef CONFIG_I2C_SLAVE
+	if (i2c_bus->mode == BUFF_MODE) {
+		if (i2c_bus->slave_operate)
+			return -EBUSY;
+		/* disable slave isr */
+		writel(0, i2c_bus->reg_base + AST2600_I2CS_IER);
+		if (readl(i2c_bus->reg_base + AST2600_I2CS_ISR) || (i2c_bus->slave_operate)) {
+			writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_IER);
+			return -EBUSY;
+		}
+	}
+#endif
+
+	i2c_bus->cmd_err = 0;
+	i2c_bus->msgs = msgs;
+	i2c_bus->msgs_index = 0;
+	i2c_bus->msgs_count = num;
+	reinit_completion(&i2c_bus->cmd_complete);
+	ret = ast2600_i2c_do_start(i2c_bus);
+#ifdef CONFIG_I2C_SLAVE
+	/* avoid race condication slave is wait and master wait 1st slave operate */
+	if (i2c_bus->mode == BUFF_MODE)
+		writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_IER);
+#endif
+	if (ret)
+		goto master_out;
+	timeout = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout);
+	if (timeout == 0) {
+		u32 isr = readl(i2c_bus->reg_base + AST2600_I2CM_ISR);
+		u32 i2c_status = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
+
+		dev_dbg(i2c_bus->dev, "timeout isr[%x], sts[%x]\n", isr, i2c_status);
+		if (isr || (i2c_status & AST2600_I2CC_TX_DIR_MASK)) {
+			u32 ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+			writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+			writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+#ifdef CONFIG_I2C_SLAVE
+			if (ctrl & AST2600_I2CC_SLAVE_EN) {
+				u32 cmd = SLAVE_TRIGGER_CMD;
+
+				if (i2c_bus->mode == DMA_MODE) {
+					cmd |= AST2600_I2CS_RX_DMA_EN;
+					writel(i2c_bus->slave_dma_addr,
+						   i2c_bus->reg_base + AST2600_I2CS_RX_DMA);
+					writel(i2c_bus->slave_dma_addr,
+						   i2c_bus->reg_base + AST2600_I2CS_TX_DMA);
+					writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
+						   i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
+				} else if (i2c_bus->mode == BUFF_MODE) {
+					cmd = SLAVE_TRIGGER_CMD;
+				} else {
+					cmd &= ~AST2600_I2CS_PKT_MODE_EN;
+				}
+				writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+			}
+#endif
+		}
+		ret = -ETIMEDOUT;
+	} else {
+		ret = i2c_bus->cmd_err;
+	}
+
+	dev_dbg(i2c_bus->dev, "bus%d-m: %d end\n", i2c_bus->adap.nr, i2c_bus->cmd_err);
+
+master_out:
+	if (i2c_bus->mode == DMA_MODE) {
+		kfree(i2c_bus->master_safe_buf);
+	    i2c_bus->master_safe_buf = NULL;
+	}
+
+	return ret;
+}
+
+static void ast2600_i2c_init(struct ast2600_i2c_bus *i2c_bus)
+{
+	struct platform_device *pdev = to_platform_device(i2c_bus->dev);
+	u32 fun_ctrl = AST2600_I2CC_BUS_AUTO_RELEASE | AST2600_I2CC_MASTER_EN;
+
+	/* I2C Reset */
+	writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+	if (of_property_read_bool(pdev->dev.of_node, "multi-master"))
+		i2c_bus->multi_master = true;
+	else
+		fun_ctrl |= AST2600_I2CC_MULTI_MASTER_DIS;
+
+	/* Enable Master Mode */
+	writel(fun_ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+	/* disable slave address */
+	writel(0, i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
+
+	/* Set AC Timing */
+	writel(ast2600_select_i2c_clock(i2c_bus), i2c_bus->reg_base + AST2600_I2CC_AC_TIMING);
+
+	/* Clear Interrupt */
+	writel(0xfffffff, i2c_bus->reg_base + AST2600_I2CM_ISR);
+
+#ifdef CONFIG_I2C_SLAVE
+	/* for memory buffer initial */
+	if (i2c_bus->mode == DMA_MODE) {
+		i2c_bus->slave_dma_buf = dma_alloc_coherent(i2c_bus->dev, I2C_SLAVE_MSG_BUF_SIZE,
+							    &i2c_bus->slave_dma_addr, GFP_KERNEL);
+		if (!i2c_bus->slave_dma_buf)
+			return;
+	}
+
+	writel(0xfffffff, i2c_bus->reg_base + AST2600_I2CS_ISR);
+
+	if (i2c_bus->mode == BYTE_MODE) {
+		writel(0xffff, i2c_bus->reg_base + AST2600_I2CS_IER);
+	} else {
+		/* Set interrupt generation of I2C slave controller */
+		writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_IER);
+	}
+#endif
+}
+
+#ifdef CONFIG_I2C_SLAVE
+static int ast2600_i2c_reg_slave(struct i2c_client *client)
+{
+	struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(client->adapter);
+	u32 cmd = SLAVE_TRIGGER_CMD;
+
+	if (i2c_bus->slave)
+		return -EINVAL;
+
+	dev_dbg(i2c_bus->dev, "slave addr %x\n", client->addr);
+
+	writel(0, i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
+	writel(AST2600_I2CC_SLAVE_EN | readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL),
+	       i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+	/* trigger rx buffer */
+	if (i2c_bus->mode == DMA_MODE) {
+		cmd |= AST2600_I2CS_RX_DMA_EN;
+		writel(i2c_bus->slave_dma_addr, i2c_bus->reg_base + AST2600_I2CS_RX_DMA);
+		writel(i2c_bus->slave_dma_addr, i2c_bus->reg_base + AST2600_I2CS_TX_DMA);
+		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
+		       i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
+	} else if (i2c_bus->mode == BUFF_MODE) {
+		cmd = SLAVE_TRIGGER_CMD;
+	} else {
+		cmd &= ~AST2600_I2CS_PKT_MODE_EN;
+	}
+
+	writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+	i2c_bus->slave = client;
+	/* Set slave addr. */
+	writel(client->addr | AST2600_I2CS_ADDR1_ENABLE,
+			i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
+
+	return 0;
+}
+
+static int ast2600_i2c_unreg_slave(struct i2c_client *slave)
+{
+	struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(slave->adapter);
+
+	WARN_ON(!i2c_bus->slave);
+
+	/* Turn off slave mode. */
+	writel(~AST2600_I2CC_SLAVE_EN & readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL),
+	       i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+	writel(readl(i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL) & ~AST2600_I2CS_ADDR1_MASK,
+	       i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
+
+	i2c_bus->slave = NULL;
+
+	return 0;
+}
+#endif
+
+static u32 ast2600_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
+}
+
+static const struct i2c_algorithm i2c_ast2600_algorithm = {
+	.master_xfer = ast2600_i2c_master_xfer,
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	.reg_slave = ast2600_i2c_reg_slave,
+	.unreg_slave = ast2600_i2c_unreg_slave,
+#endif
+	.functionality = ast2600_i2c_functionality,
+};
+
+static const struct of_device_id ast2600_i2c_bus_of_table[] = {
+	{
+		.compatible = "aspeed,ast2600-i2cv2",
+	},
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, ast2600_i2c_bus_of_table);
+
+static int ast2600_xfer_mode_str2val(const char *str,
+			       const struct ast2600_xfer_mode_str2field *list)
+{
+	const struct ast2600_xfer_mode_str2field *p = list;
+
+	for (p = list; p && p->name; p++)
+		if (!strcmp(p->name, str))
+			return p->mode;
+
+	/* default dma mode */
+	return DMA_MODE;
+}
+
+static int ast2600_i2c_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct ast2600_i2c_bus *i2c_bus;
+	struct resource *res;
+	const char *of_str;
+	u32 global_ctrl;
+	int ret = 0;
+
+	i2c_bus = devm_kzalloc(dev, sizeof(*i2c_bus), GFP_KERNEL);
+	if (!i2c_bus)
+		return -ENOMEM;
+
+	i2c_bus->reg_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(i2c_bus->reg_base))
+		return PTR_ERR(i2c_bus->reg_base);
+
+	i2c_bus->rst = devm_reset_control_get_shared(dev, NULL);
+	if (IS_ERR(i2c_bus->rst))
+		return dev_err_probe(dev, PTR_ERR(i2c_bus->rst), "Missing reset ctrl\n");
+
+	reset_control_deassert(i2c_bus->rst);
+
+	i2c_bus->global_regs = syscon_regmap_lookup_by_phandle(np, "aspeed,global-regs");
+	if (IS_ERR(i2c_bus->global_regs))
+		return PTR_ERR(i2c_bus->global_regs);
+
+	regmap_read(i2c_bus->global_regs, AST2600_I2CG_CTRL, &global_ctrl);
+	if ((global_ctrl & AST2600_GLOBAL_INIT) != AST2600_GLOBAL_INIT) {
+		regmap_write(i2c_bus->global_regs, AST2600_I2CG_CTRL, AST2600_GLOBAL_INIT);
+		regmap_write(i2c_bus->global_regs, AST2600_I2CG_CLK_DIV_CTRL, I2CCG_DIV_CTRL);
+	}
+
+	i2c_bus->timeout_enable = 0;
+	i2c_bus->slave_operate = 0;
+	i2c_bus->dev = dev;
+
+	ret = of_property_read_string_index(dev->of_node,
+						"aspeed,xfer-mode", 0,
+						&of_str);
+	if (!ret)
+		i2c_bus->mode = ast2600_xfer_mode_str2val(of_str, ast2600_xfer_mode_type);
+	else
+		i2c_bus->mode = DMA_MODE;
+
+
+	if (i2c_bus->mode == BUFF_MODE) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		if (res && resource_size(res) >= 2)
+			i2c_bus->buf_base = devm_ioremap_resource(dev, res);
+
+		if (!IS_ERR_OR_NULL(i2c_bus->buf_base))
+			i2c_bus->buf_size = resource_size(res)/2;
+	}
+
+	if (of_property_read_bool(dev->of_node, "aspeed,timeout"))
+		i2c_bus->timeout_enable = 1;
+
+	init_completion(&i2c_bus->cmd_complete);
+
+	i2c_bus->irq = platform_get_irq(pdev, 0);
+	if (i2c_bus->irq < 0)
+		return i2c_bus->irq;
+
+	platform_set_drvdata(pdev, i2c_bus);
+
+	i2c_bus->clk = devm_clk_get(i2c_bus->dev, NULL);
+	if (IS_ERR(i2c_bus->clk))
+		return dev_err_probe(i2c_bus->dev, PTR_ERR(i2c_bus->clk), "Can't get clock\n");
+
+	i2c_bus->apb_clk = clk_get_rate(i2c_bus->clk);
+
+	ret = of_property_read_u32(dev->of_node, "clock-frequency", &i2c_bus->bus_frequency);
+	if (ret < 0) {
+		dev_warn(dev, "Could not read clock-frequency property\n");
+		i2c_bus->bus_frequency = 100000;
+	}
+
+	/* Initialize the I2C adapter */
+	i2c_bus->adap.owner = THIS_MODULE;
+	i2c_bus->adap.algo = &i2c_ast2600_algorithm;
+	i2c_bus->adap.retries = 0;
+	i2c_bus->adap.dev.parent = i2c_bus->dev;
+	i2c_bus->adap.dev.of_node = dev->of_node;
+	i2c_bus->adap.algo_data = i2c_bus;
+	strscpy(i2c_bus->adap.name, pdev->name, sizeof(i2c_bus->adap.name));
+	i2c_set_adapdata(&i2c_bus->adap, i2c_bus);
+
+	ast2600_i2c_init(i2c_bus);
+
+	ret = devm_request_irq(dev, i2c_bus->irq, ast2600_i2c_bus_irq, 0,
+			       dev_name(dev), i2c_bus);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Unable to request irq %d\n", i2c_bus->irq);
+
+	if (of_property_read_bool(dev->of_node, "smbus-alert")) {
+		i2c_bus->alert_enable = 1;
+		i2c_bus->ara = i2c_new_smbus_alert_device(&i2c_bus->adap, &i2c_bus->alert_data);
+		if (!i2c_bus->ara)
+			dev_warn(dev, "Failed to register ARA client\n");
+
+		writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER | AST2600_I2CM_SMBUS_ALT,
+		       i2c_bus->reg_base + AST2600_I2CM_IER);
+	} else {
+		i2c_bus->alert_enable = 0;
+		/* Set interrupt generation of I2C master controller */
+		writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
+				i2c_bus->reg_base + AST2600_I2CM_IER);
+	}
+
+	ret = i2c_add_adapter(&i2c_bus->adap);
+	if (ret)
+		return ret;
+
+	dev_info(dev, "%s [%d]: adapter [%d khz] mode [%d]\n",
+		 dev->of_node->name, i2c_bus->adap.nr, i2c_bus->bus_frequency / 1000,
+		 i2c_bus->mode);
+
+	return 0;
+
+	return ret;
+}
+
+static int ast2600_i2c_remove(struct platform_device *pdev)
+{
+	struct ast2600_i2c_bus *i2c_bus = platform_get_drvdata(pdev);
+
+	/* Disable everything. */
+	writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+	writel(0, i2c_bus->reg_base + AST2600_I2CM_IER);
+
+	i2c_del_adapter(&i2c_bus->adap);
+	devm_free_irq(&pdev->dev, i2c_bus->irq, i2c_bus);
+
+#ifdef CONFIG_I2C_SLAVE
+	/* for memory buffer initial */
+	if (i2c_bus->mode == DMA_MODE)
+		dma_free_coherent(i2c_bus->dev, I2C_SLAVE_MSG_BUF_SIZE,
+				i2c_bus->slave_dma_buf, i2c_bus->slave_dma_addr);
+#endif
+
+	return 0;
+}
+
+static struct platform_driver ast2600_i2c_bus_driver = {
+	.probe = ast2600_i2c_probe,
+	.remove = ast2600_i2c_remove,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = ast2600_i2c_bus_of_table,
+	},
+};
+module_platform_driver(ast2600_i2c_bus_driver);
+
+MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
+MODULE_DESCRIPTION("ASPEED AST2600 I2C Controller Driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-02-26  3:13 ` [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2 Ryan Chen
@ 2023-02-26  7:04   ` Jeremy Kerr
  2023-02-27  4:12     ` Ryan Chen
  2023-02-27  8:25   ` Krzysztof Kozlowski
  2023-03-18  9:09   ` Andi Shyti
  2 siblings, 1 reply; 28+ messages in thread
From: Jeremy Kerr @ 2023-02-26  7:04 UTC (permalink / raw)
  To: Ryan Chen, Andrew Jeffery, Brendan Higgins,
	Benjamin Herrenschmidt, Joel Stanley, Rob Herring,
	Krzysztof Kozlowski, Philipp Zabel, linux-i2c, openbmc,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel

Hi Ryan,

> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> @@ -49,6 +49,25 @@ properties:
>      description:
>        states that there is another master active on this bus
>  
> +  aspeed,timeout:
> +    type: boolean
> +    description: I2C bus timeout enable for master/slave mode
> +
> +  aspeed,xfer-mode:
> +    description: |
> +      I2C bus transfer mode selection.
> +      - "byte": I2C bus byte transfer mode.
> +      - "buffered": I2C bus buffer register transfer mode.
> +      - "dma": I2C bus dma transfer mode (default)
> +    items:
> +      enum: [byte, buffered, dma]
> +    maxItems: 1
> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array

There are still unresolved questions about this xfer-mode property from
previous submissions of this binding. We don't yet have a justification
on why the mode configuration is needed in the device tree rather than
something that is specified in a driver implementation.

By now, I think we well understand what the modes are, and how a driver
implementation might configure them, but none of that has (so far)
provided sufficient rationale on why this belongs in the device tree.

The previous threads had a couple of pending discussions, following up on
those here:

A) You mentioned in [1] that the DMA controller is shared between all i3c
devices, does that have any consequence on which modes individual
devices might want to choose?

B) You implied in [2] that the different transfer modes might be related
to whether there are other masters present on the bus, but the logic
behind that is not clear.

C) In [3] you mentioned that there might be some DRAM savings by using a
particular mode.

and, most importantly:

D) unanswered from [4] and [5]: what are the hardware-specified reasons
why a DT author would chose one mode over another?

If you can write this out in some format like:

 - in hardware situation X, you should use DMA mode
 - in hardware situation Y, you should use byte mode
 - [...]

that might help us to understand where this configuration belongs, or
what a reasonable DT representation should look like, or even if
existing DT schema can already provide the information required to
decide.

Cheers,


Jeremy

[1]: https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009876.html
[2]: https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009892.html
[3]: https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009880.html
[4]: https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009871.html
[5]: https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009884.html

^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-02-26  7:04   ` Jeremy Kerr
@ 2023-02-27  4:12     ` Ryan Chen
  2023-02-27  5:40       ` Jeremy Kerr
  0 siblings, 1 reply; 28+ messages in thread
From: Ryan Chen @ 2023-02-27  4:12 UTC (permalink / raw)
  To: Jeremy Kerr, Andrew Jeffery, Brendan Higgins,
	Benjamin Herrenschmidt, Joel Stanley, Rob Herring,
	Krzysztof Kozlowski, Philipp Zabel, linux-i2c, openbmc,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel

Hello Jeremy,

> -----Original Message-----
> From: Jeremy Kerr <jk@codeconstruct.com.au>
> Sent: Sunday, February 26, 2023 3:04 PM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Andrew Jeffery
> <andrew@aj.id.au>; Brendan Higgins <brendan.higgins@linux.dev>; Benjamin
> Herrenschmidt <benh@kernel.crashing.org>; Joel Stanley <joel@jms.id.au>;
> Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Philipp Zabel <p.zabel@pengutronix.de>;
> linux-i2c@vger.kernel.org; openbmc@lists.ozlabs.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> Hi Ryan,
> 
> > --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > @@ -49,6 +49,25 @@ properties:
> >      description:
> >        states that there is another master active on this bus
> >
> > +  aspeed,timeout:
> > +    type: boolean
> > +    description: I2C bus timeout enable for master/slave mode
> > +
> > +  aspeed,xfer-mode:
> > +    description: |
> > +      I2C bus transfer mode selection.
> > +      - "byte": I2C bus byte transfer mode.
> > +      - "buffered": I2C bus buffer register transfer mode.
> > +      - "dma": I2C bus dma transfer mode (default)
> > +    items:
> > +      enum: [byte, buffered, dma]
> > +    maxItems: 1
> > +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> 
> There are still unresolved questions about this xfer-mode property from
> previous submissions of this binding. We don't yet have a justification on why
> the mode configuration is needed in the device tree rather than something
> that is specified in a driver implementation.
> 
> By now, I think we well understand what the modes are, and how a driver
> implementation might configure them, but none of that has (so far) provided
> sufficient rationale on why this belongs in the device tree.
> 
> The previous threads had a couple of pending discussions, following up on
> those here:
> 
> A) You mentioned in [1] that the DMA controller is shared between all i3c
> devices, does that have any consequence on which modes individual devices
> might want to choose?

Yes, I2C controller share the same dma engine. The original thought can be enable in
all i2c channel. But in AST2600 have ERRATA "I2C DMA fails when DRAM bus is busy
and it can not take DMA write data immediately", So it means only 1 i2c bus can be
enable for DMA mode.
It means only 1 bus channel can be enable DMA for use case.
That following example for board-specific selection.
It is description in cover-letter.

The following is board-specific design example.
Board A                                         Board B
-------------------------                       ------------------------
|i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
|i2c bus#2(master)-> tmp i2c device |        |                   |
|i2c bus#3(master)-> adc i2c device |        |                   |
-------------------------                       ------------------------

- in bus#1 situation, you should use DMA mode.
Because bus#1 have trunk data needed for transfer, it can enable bus dma mode to reduce cpu utilized.
- in bus#2/3 situation, you should use buffer/byte mode
bus#2/3 is small package transmit, it can enable buffer mode or byte mode to reduce memory cache flush overhead.
Buffer mode is better, because byte mode have interrupt overhead(interrupt per byte data transmit),

-But if you more bus#4 that still have trunk data needed for transfer (master/slave),
it also use buffer mode to transmit. Because bus#1 have been use for DMA mode.

Best Regards.
Ryan Chen.

> 
> B) You implied in [2] that the different transfer modes might be related to
> whether there are other masters present on the bus, but the logic behind that
> is not clear.
> 
> C) In [3] you mentioned that there might be some DRAM savings by using a
> particular mode.
> 
> and, most importantly:
> 
> D) unanswered from [4] and [5]: what are the hardware-specified reasons why
> a DT author would chose one mode over another?
> 
> If you can write this out in some format like:
> 
>  - in hardware situation X, you should use DMA mode
>  - in hardware situation Y, you should use byte mode
>  - [...]
> 
> that might help us to understand where this configuration belongs, or what a
> reasonable DT representation should look like, or even if existing DT schema
> can already provide the information required to decide.
> 
> Cheers,
> 
> 
> Jeremy
> 
> [1]:
> https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009876.html
> [2]:
> https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009892.html
> [3]:
> https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009880.html
> [4]:
> https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009871.html
> [5]:
> https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009884.html

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-02-27  4:12     ` Ryan Chen
@ 2023-02-27  5:40       ` Jeremy Kerr
  2023-03-01  3:02         ` Ryan Chen
  0 siblings, 1 reply; 28+ messages in thread
From: Jeremy Kerr @ 2023-02-27  5:40 UTC (permalink / raw)
  To: Ryan Chen, Andrew Jeffery, Brendan Higgins,
	Benjamin Herrenschmidt, Joel Stanley, Rob Herring,
	Krzysztof Kozlowski, Philipp Zabel, linux-i2c, openbmc,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel

Hi Ryan,

> Yes, I2C controller share the same dma engine. The original thought
> can be enable in all i2c channel. But in AST2600 have ERRATA "I2C DMA
> fails when DRAM bus is busy and it can not take DMA write data
> immediately", So it means only 1 i2c bus can be enable for DMA mode.

OK, this is a pretty important detail! I'd suggest putting it in the
binding document.

Anything in the cover letter will get lost after review. If there is
documentation that would be useful for a DTS author, I'd suggest putting
it in the binding.

> It means only 1 bus channel can be enable DMA for use case.
> That following example for board-specific selection.
> It is description in cover-letter.
> The following is board-specific design example.
> Board A                                         Board B
> -------------------------                       ------------------------
> > i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
> > i2c bus#2(master)-> tmp i2c device |        |                   |
> > i2c bus#3(master)-> adc i2c device |        |                   |
> -------------------------                       ------------------------
> 
> - in bus#1 situation, you should use DMA mode.
> Because bus#1 have trunk data needed for transfer, it can enable bus
> dma mode to reduce cpu utilized.

What is "trunk data" in this context? Is this just a statement about the
amount of expected transfers?

> - in bus#2/3 situation, you should use buffer/byte mode
> bus#2/3 is small package transmit, it can enable buffer mode or byte
> mode to reduce memory cache flush overhead.
> Buffer mode is better, because byte mode have interrupt
> overhead(interrupt per byte data transmit),
> 
> -But if you more bus#4 that still have trunk data needed for transfer
> (master/slave),
> it also use buffer mode to transmit. Because bus#1 have been use for
> DMA mode.

So, it sounds like:

 - there's no point in using byte mode, as buffer mode provides
   equivalent functionality with fewer drawbacks (ie, less interrupt
   load)

 - this just leaves the dma and buffer modes

 - only one controller can use dma mode

So: how about just a single boolean property to indicate "use DMA on
this controller"? Something like aspeed,enable-dma? Or if DT binding
experts can suggest something common that might be more suitable?

Cheers,


Jeremy

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-02-26  3:13 ` [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2 Ryan Chen
  2023-02-26  7:04   ` Jeremy Kerr
@ 2023-02-27  8:25   ` Krzysztof Kozlowski
  2023-03-01  5:57     ` Ryan Chen
  2023-03-18  9:09   ` Andi Shyti
  2 siblings, 1 reply; 28+ messages in thread
From: Krzysztof Kozlowski @ 2023-02-27  8:25 UTC (permalink / raw)
  To: Ryan Chen, Andrew Jeffery, Brendan Higgins,
	Benjamin Herrenschmidt, Joel Stanley, Rob Herring,
	Krzysztof Kozlowski, Philipp Zabel, linux-i2c, openbmc,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel

On 26/02/2023 04:13, Ryan Chen wrote:
> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout
> aspeed,xfer-mode description for ast2600-i2cv2.
> 
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> index f597f73ccd87..75de3ce41cf5 100644
> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> @@ -49,6 +49,25 @@ properties:
>      description:
>        states that there is another master active on this bus
>  
> +  aspeed,timeout:
> +    type: boolean
> +    description: I2C bus timeout enable for master/slave mode

Nothing improved here in regards to my last comment.

> +
> +  aspeed,xfer-mode:
> +    description: |
> +      I2C bus transfer mode selection.
> +      - "byte": I2C bus byte transfer mode.
> +      - "buffered": I2C bus buffer register transfer mode.
> +      - "dma": I2C bus dma transfer mode (default)
> +    items:
> +      enum: [byte, buffered, dma]
> +    maxItems: 1

Drop, not an array.

> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array

Wrong ref. This is not an array, but one string.

> +
> +  aspeed,global-regs:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: The phandle of i2c global register node.
> +
>  required:
>    - reg
>    - compatible
> @@ -57,6 +76,19 @@ required:
>  
>  unevaluatedProperties: false
>  
> +if:

This should be under allOf (in this location)

> +  properties:
> +    compatible:
> +      contains:
> +        const: aspeed,ast2600-i2cv2
> +
> +then:
> +  properties:
> +    reg:
> +      minItems: 2
> +  required:
> +    - aspeed,global-regs

else:
  aspeed,global-regs: false
and the same for other v2 properties

> +
>  examples:
>    - |
>      #include <dt-bindings/clock/aspeed-clock.h>
> @@ -71,3 +103,15 @@ examples:
>        interrupts = <0>;
>        interrupt-parent = <&i2c_ic>;
>      };
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    i2c1: i2c@80 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      compatible = "aspeed,ast2600-i2cv2";
> +      reg = <0x80 0x80>, <0xc00 0x20>;
> +      interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
> +      aspeed,global-regs = <&i2c_global>;
> +      clocks = <&syscon ASPEED_CLK_APB>;
> +      resets = <&syscon ASPEED_RESET_I2C>;
> +    };

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-02-27  5:40       ` Jeremy Kerr
@ 2023-03-01  3:02         ` Ryan Chen
  2023-03-01  3:23           ` Jeremy Kerr
  0 siblings, 1 reply; 28+ messages in thread
From: Ryan Chen @ 2023-03-01  3:02 UTC (permalink / raw)
  To: Jeremy Kerr, Andrew Jeffery, Brendan Higgins,
	Benjamin Herrenschmidt, Joel Stanley, Rob Herring,
	Krzysztof Kozlowski, Philipp Zabel, linux-i2c, openbmc,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel

Hello Jeremy,

> -----Original Message-----
> From: Jeremy Kerr <jk@codeconstruct.com.au>
> Sent: Monday, February 27, 2023 1:40 PM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Andrew Jeffery
> <andrew@aj.id.au>; Brendan Higgins <brendan.higgins@linux.dev>; Benjamin
> Herrenschmidt <benh@kernel.crashing.org>; Joel Stanley <joel@jms.id.au>;
> Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Philipp Zabel <p.zabel@pengutronix.de>;
> linux-i2c@vger.kernel.org; openbmc@lists.ozlabs.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> Hi Ryan,
> 
> > Yes, I2C controller share the same dma engine. The original thought
> > can be enable in all i2c channel. But in AST2600 have ERRATA "I2C DMA
> > fails when DRAM bus is busy and it can not take DMA write data
> > immediately", So it means only 1 i2c bus can be enable for DMA mode.
> 
> OK, this is a pretty important detail! I'd suggest putting it in the binding
> document.

Sorry, Do you mean add in description like following??
  aspeed,xfer-mode:
    description: |
      I2C bus transfer mode selection.
	  ERRATA "I2C DMA fails when DRAM bus is busy and it can not take DMA write data
Immediately", only 1 i2c bus can be enable for DMA mode.
      - "byte": I2C bus byte transfer mode.
      - "buffered": I2C bus buffer register transfer mode.
      - "dma": I2C bus dma transfer mode (default)
      
> Anything in the cover letter will get lost after review. If there is documentation
> that would be useful for a DTS author, I'd suggest putting it in the binding.
> 
> > It means only 1 bus channel can be enable DMA for use case.
> > That following example for board-specific selection.
> > It is description in cover-letter.
> > The following is board-specific design example.
> > Board A                                         Board
> B
> > -------------------------
> > ------------------------
> > > i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x
> > > (master/slave)| i2c bus#2(master)-> tmp i2c device
> |        |
> > > | i2c bus#3(master)-> adc i2c device
> |        |                   |
> > -------------------------
> > ------------------------
> >
> > - in bus#1 situation, you should use DMA mode.
> > Because bus#1 have trunk data needed for transfer, it can enable bus
> > dma mode to reduce cpu utilized.
> 
> What is "trunk data" in this context? Is this just a statement about the amount
> of expected transfers?
Sorry, I can't catch your point, for example for most server application usage.
The i2c not only connect with small device (like temperature sensor/ adc).
It also connect with other mcu base device support i2c slave.
Most case is transfer MCTP package. (basic 64kbytes). So I say "trunk data".

> 
> > - in bus#2/3 situation, you should use buffer/byte mode
> > bus#2/3 is small package transmit, it can enable buffer mode or byte
> > mode to reduce memory cache flush overhead.
> > Buffer mode is better, because byte mode have interrupt
> > overhead(interrupt per byte data transmit),
> >
> > -But if you more bus#4 that still have trunk data needed for transfer
> > (master/slave), it also use buffer mode to transmit. Because bus#1
> > have been use for DMA mode.
> 
> So, it sounds like:
> 
>  - there's no point in using byte mode, as buffer mode provides
>    equivalent functionality with fewer drawbacks (ie, less interrupt
>    load)
> 
>  - this just leaves the dma and buffer modes
> 
>  - only one controller can use dma mode
> 
> So: how about just a single boolean property to indicate "use DMA on this
> controller"? Something like aspeed,enable-dma? Or if DT binding experts can
> suggest something common that might be more suitable?

If so, just leave enable-dma and only support for buffer mode and dma mode, am I right?

Best Regards.
Ryan

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-01  3:02         ` Ryan Chen
@ 2023-03-01  3:23           ` Jeremy Kerr
  2023-03-01  3:40             ` Ryan Chen
  0 siblings, 1 reply; 28+ messages in thread
From: Jeremy Kerr @ 2023-03-01  3:23 UTC (permalink / raw)
  To: Ryan Chen, Andrew Jeffery, Brendan Higgins,
	Benjamin Herrenschmidt, Joel Stanley, Rob Herring,
	Krzysztof Kozlowski, Philipp Zabel, linux-i2c, openbmc,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel

Hi Ryan,

> Sorry, Do you mean add in description like following??
>   aspeed,xfer-mode:
>     description: |
>       I2C bus transfer mode selection.
>           ERRATA "I2C DMA fails when DRAM bus is busy and it can not
> take DMA write data
> Immediately", only 1 i2c bus can be enable for DMA mode.
>       - "byte": I2C bus byte transfer mode.
>       - "buffered": I2C bus buffer register transfer mode.
>       - "dma": I2C bus dma transfer mode (default)

I would suggest putting some background about the transfer mode as a
top-level description in the binding.

There has been a lot of discussion here on why the binding specifies
the transfer mode; it would be useful (for future readers) to have a
bit of context on what modes they should use.

Perhaps something like:

    description: |
      [general binding description]

      ASPEED ast2600 platforms have a number of i2c controllers, and share a
      single DMA engine between the set. DTSes can specify the mode of data
      transfer to/from the device - either DMA or programmed I/O - but
      hardware limitations may require a DTS to manually allocate which
      controller can use DMA mode; the enable-dma property allows control of
      this.

      In cases where one the hardware design results in a specific
      controller handling a larger amount of data, a DTS would likely
      allocate DMA mode for that one controller.

- adjusted for whatever property interface we settle on here, of course.

> > So, it sounds like:
> > 
> >  - there's no point in using byte mode, as buffer mode provides
> >    equivalent functionality with fewer drawbacks (ie, less interrupt
> >    load)
> > 
> >  - this just leaves the dma and buffer modes
> > 
> >  - only one controller can use dma mode
> > 
> > So: how about just a single boolean property to indicate "use DMA
> > on this controller"? Something like aspeed,enable-dma? Or if DT binding
> > experts can suggest something common that might be more suitable?
> 
> If so, just leave enable-dma and only support for buffer mode and dma
> mode, am I right?

Yes, from what you have said so far, I think just a single switch
between DMA / not-DMA is all you need here (unless there is any time
that byte mode is preferable?)

If there is already an existing DT convention for indicating/enabling
DMA capability, I would suggest using that. Otherwise, just a boolean
flag with a sensible name would seem to work fine. The DT experts
probably have a good idea of what works best here :)

Cheers,


Jeremy

^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-01  3:23           ` Jeremy Kerr
@ 2023-03-01  3:40             ` Ryan Chen
  0 siblings, 0 replies; 28+ messages in thread
From: Ryan Chen @ 2023-03-01  3:40 UTC (permalink / raw)
  To: Jeremy Kerr, Andrew Jeffery, Brendan Higgins,
	Benjamin Herrenschmidt, Joel Stanley, Rob Herring,
	Krzysztof Kozlowski, Philipp Zabel, linux-i2c, openbmc,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel

Hello Jeremy,

> -----Original Message-----
> From: Jeremy Kerr <jk@codeconstruct.com.au>
> Sent: Wednesday, March 1, 2023 11:24 AM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Andrew Jeffery
> <andrew@aj.id.au>; Brendan Higgins <brendan.higgins@linux.dev>; Benjamin
> Herrenschmidt <benh@kernel.crashing.org>; Joel Stanley <joel@jms.id.au>;
> Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Philipp Zabel <p.zabel@pengutronix.de>;
> linux-i2c@vger.kernel.org; openbmc@lists.ozlabs.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> Hi Ryan,
> 
> > Sorry, Do you mean add in description like following??
> >   aspeed,xfer-mode:
> >     description: |
> >       I2C bus transfer mode selection.
> >           ERRATA "I2C DMA fails when DRAM bus is busy and it can
> not
> > take DMA write data Immediately", only 1 i2c bus can be enable for DMA
> > mode.
> >       - "byte": I2C bus byte transfer mode.
> >       - "buffered": I2C bus buffer register transfer mode.
> >       - "dma": I2C bus dma transfer mode (default)
> 
> I would suggest putting some background about the transfer mode as a
> top-level description in the binding.
> 
> There has been a lot of discussion here on why the binding specifies the
> transfer mode; it would be useful (for future readers) to have a bit of context
> on what modes they should use.
> 
> Perhaps something like:
> 
>     description: |
>       [general binding description]
> 
>       ASPEED ast2600 platforms have a number of i2c controllers, and share
> a
>       single DMA engine between the set. DTSes can specify the mode of
> data
>       transfer to/from the device - either DMA or programmed I/O - but
>       hardware limitations may require a DTS to manually allocate which
>       controller can use DMA mode; the enable-dma property allows control
> of
>       this.
> 
>       In cases where one the hardware design results in a specific
>       controller handling a larger amount of data, a DTS would likely
>       allocate DMA mode for that one controller.
> 
> - adjusted for whatever property interface we settle on here, of course.
> 
It is more clear now, I will add in next patch.

> > > So, it sounds like:
> > >
> > >  - there's no point in using byte mode, as buffer mode provides
> > >    equivalent functionality with fewer drawbacks (ie, less interrupt
> > >    load)
> > >
> > >  - this just leaves the dma and buffer modes
> > >
> > >  - only one controller can use dma mode
> > >
> > > So: how about just a single boolean property to indicate "use DMA on
> > > this controller"? Something like aspeed,enable-dma? Or if DT binding
> > > experts can suggest something common that might be more suitable?
> >
> > If so, just leave enable-dma and only support for buffer mode and dma
> > mode, am I right?
> 
> Yes, from what you have said so far, I think just a single switch between DMA /
> not-DMA is all you need here (unless there is any time that byte mode is
> preferable?)

Yes, I also think so, but if I only for dma to be single Boolean property.
Should I remove all byte mode capability in driver?

Best Regards.
Ryan

> If there is already an existing DT convention for indicating/enabling DMA
> capability, I would suggest using that. Otherwise, just a boolean flag with a
> sensible name would seem to work fine. The DT experts probably have a good
> idea of what works best here :)
> 
> Cheers,
> 
> 
> Jeremy

^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-02-27  8:25   ` Krzysztof Kozlowski
@ 2023-03-01  5:57     ` Ryan Chen
  2023-03-03  8:20       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 28+ messages in thread
From: Ryan Chen @ 2023-03-01  5:57 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Andrew Jeffery, Brendan Higgins,
	Benjamin Herrenschmidt, Joel Stanley, Rob Herring,
	Krzysztof Kozlowski, Philipp Zabel, linux-i2c, openbmc,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Monday, February 27, 2023 4:25 PM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Andrew Jeffery
> <andrew@aj.id.au>; Brendan Higgins <brendan.higgins@linux.dev>; Benjamin
> Herrenschmidt <benh@kernel.crashing.org>; Joel Stanley <joel@jms.id.au>;
> Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Philipp Zabel <p.zabel@pengutronix.de>;
> linux-i2c@vger.kernel.org; openbmc@lists.ozlabs.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> On 26/02/2023 04:13, Ryan Chen wrote:
> > Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout
> > aspeed,xfer-mode description for ast2600-i2cv2.
> >
> > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> > ---
> >  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > index f597f73ccd87..75de3ce41cf5 100644
> > --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > @@ -49,6 +49,25 @@ properties:
> >      description:
> >        states that there is another master active on this bus
> >
> > +  aspeed,timeout:
> > +    type: boolean
> > +    description: I2C bus timeout enable for master/slave mode
> 
> Nothing improved here in regards to my last comment.

Yes, as I know your require is about " DT binding to represent hardware setup"
So I add more description about aspeed,timeout as blow.

ASPEED SOC chip is server product, i2c bus may have fingerprint connect to another board. And also support hotplug.
The following is board-specific design example.
Board A                                         Board B
-------------------------                       ------------------------
|i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
|i2c bus#2(master)-> tmp i2c device |          |                       |
|i2c bus#3(master)-> adc i2c device |          |                       |
-------------------------                       ------------------------

aspeed,timout properites:
For example I2C controller as slave mode, and suddenly disconnected.
Slave state machine will keep waiting for master clock in for rx/tx transmit.
So it need timeout setting to enable timeout unlock controller state.
And in another side. In Master side also need avoid suddenly slave miss(un-plug), Master will timeout and release the SDA/SCL.

Do you mean add those description into ore aspeed,timout properites description?

> 
> > +
> > +  aspeed,xfer-mode:
> > +    description: |
> > +      I2C bus transfer mode selection.
> > +      - "byte": I2C bus byte transfer mode.
> > +      - "buffered": I2C bus buffer register transfer mode.
> > +      - "dma": I2C bus dma transfer mode (default)
> > +    items:
> > +      enum: [byte, buffered, dma]
> > +    maxItems: 1
> 
> Drop, not an array.
> 
> > +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> 
> Wrong ref. This is not an array, but one string.

Sorry, I can't catch your "one string" point.
Could you point me what ref I can refer to?
That I can check into Linux example. Thanks a lot.
> 
> > +
> > +  aspeed,global-regs:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: The phandle of i2c global register node.
> > +
> >  required:
> >    - reg
> >    - compatible
> > @@ -57,6 +76,19 @@ required:
> >
> >  unevaluatedProperties: false
> >
> > +if:
> 
> This should be under allOf (in this location)
> 
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        const: aspeed,ast2600-i2cv2
> > +
> > +then:
> > +  properties:
> > +    reg:
> > +      minItems: 2
> > +  required:
> > +    - aspeed,global-regs
> 
> else:
>   aspeed,global-regs: false
> and the same for other v2 properties
> 

Does modify by following? 

allOf:
 -if:
   properties:
    compatible:
      contains:
        const: aspeed,ast2600-i2cv2

 then:
  properties:
    reg:
      minItems: 2
  required:
    - aspeed,global-regs
 else:
    - aspeed,global-regs: false
    -aspeed,timeout: false
    - aspeed,xfer-mode: false

> > +
> >  examples:
> >    - |
> >      #include <dt-bindings/clock/aspeed-clock.h>
> > @@ -71,3 +103,15 @@ examples:
> >        interrupts = <0>;
> >        interrupt-parent = <&i2c_ic>;
> >      };
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    i2c1: i2c@80 {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +      compatible = "aspeed,ast2600-i2cv2";
> > +      reg = <0x80 0x80>, <0xc00 0x20>;
> > +      interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
> > +      aspeed,global-regs = <&i2c_global>;
> > +      clocks = <&syscon ASPEED_CLK_APB>;
> > +      resets = <&syscon ASPEED_RESET_I2C>;
> > +    };
> 
Best regards,
Ryan Chen.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-01  5:57     ` Ryan Chen
@ 2023-03-03  8:20       ` Krzysztof Kozlowski
  2023-03-03  8:28         ` Ryan Chen
  2023-03-12 12:33         ` Andi Shyti
  0 siblings, 2 replies; 28+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-03  8:20 UTC (permalink / raw)
  To: Ryan Chen, Wolfram Sang
  Cc: Joel Stanley, Brendan Higgins, Krzysztof Kozlowski,
	Andrew Jeffery, devicetree, Philipp Zabel, Rob Herring,
	Benjamin Herrenschmidt, linux-aspeed, linux-arm-kernel,
	linux-kernel, openbmc, linux-i2c

On 01/03/2023 06:57, Ryan Chen wrote:
> Hello Krzysztof,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Monday, February 27, 2023 4:25 PM
>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Andrew Jeffery
>> <andrew@aj.id.au>; Brendan Higgins <brendan.higgins@linux.dev>; Benjamin
>> Herrenschmidt <benh@kernel.crashing.org>; Joel Stanley <joel@jms.id.au>;
>> Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; Philipp Zabel <p.zabel@pengutronix.de>;
>> linux-i2c@vger.kernel.org; openbmc@lists.ozlabs.org;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
>>
>> On 26/02/2023 04:13, Ryan Chen wrote:
>>> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout
>>> aspeed,xfer-mode description for ast2600-i2cv2.
>>>
>>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
>>> ---
>>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
>>>  1 file changed, 44 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> index f597f73ccd87..75de3ce41cf5 100644
>>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> @@ -49,6 +49,25 @@ properties:
>>>      description:
>>>        states that there is another master active on this bus
>>>
>>> +  aspeed,timeout:
>>> +    type: boolean
>>> +    description: I2C bus timeout enable for master/slave mode
>>
>> Nothing improved here in regards to my last comment.
> 
> Yes, as I know your require is about " DT binding to represent hardware setup"
> So I add more description about aspeed,timeout as blow.
> 
> ASPEED SOC chip is server product, i2c bus may have fingerprint connect to another board. And also support hotplug.
> The following is board-specific design example.
> Board A                                         Board B
> -------------------------                       ------------------------
> |i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
> |i2c bus#2(master)-> tmp i2c device |          |                       |
> |i2c bus#3(master)-> adc i2c device |          |                       |
> -------------------------                       ------------------------
> 
> aspeed,timout properites:
> For example I2C controller as slave mode, and suddenly disconnected.
> Slave state machine will keep waiting for master clock in for rx/tx transmit.
> So it need timeout setting to enable timeout unlock controller state.
> And in another side. In Master side also need avoid suddenly slave miss(un-plug), Master will timeout and release the SDA/SCL.
> 
> Do you mean add those description into ore aspeed,timout properites description?

You are describing here one particular feature you want to enable in the
driver which looks non-scalable and more difficult to configure/use.
What I was looking for is to describe the actual configuration you have
(e.g. multi-master) which leads to enable or disable such feature in
your hardware. Especially that bool value does not scale later to actual
timeout values in time (ms)...

I don't know I2C that much, but I wonder - why this should be specific
to Aspeed I2C and no other I2C controllers implement it? IOW, this looks
quite generic and every I2C controller should have it. Adding it
specific to Aspeed suggests that either we miss a generic property or
this should not be in DT at all (because no one else has it...).

Also I wonder, why you wouldn't enable timeout always...

+Cc Wolfram,
Maybe you know whether bool "timeout" property for one controller makes
sense? Why we do not have it for all controllers?

> 
>>
>>> +
>>> +  aspeed,xfer-mode:
>>> +    description: |
>>> +      I2C bus transfer mode selection.
>>> +      - "byte": I2C bus byte transfer mode.
>>> +      - "buffered": I2C bus buffer register transfer mode.
>>> +      - "dma": I2C bus dma transfer mode (default)
>>> +    items:
>>> +      enum: [byte, buffered, dma]
>>> +    maxItems: 1
>>
>> Drop, not an array.
>>
>>> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
>>
>> Wrong ref. This is not an array, but one string.
> 
> Sorry, I can't catch your "one string" point.

How many strings you are going to have in this property? If one
(maxItems: 1), then this is not an array.

> Could you point me what ref I can refer to?
> That I can check into Linux example. Thanks a lot.
>>
>>> +
>>> +  aspeed,global-regs:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: The phandle of i2c global register node.
>>> +
>>>  required:
>>>    - reg
>>>    - compatible
>>> @@ -57,6 +76,19 @@ required:
>>>
>>>  unevaluatedProperties: false
>>>
>>> +if:
>>
>> This should be under allOf (in this location)
>>
>>> +  properties:
>>> +    compatible:
>>> +      contains:
>>> +        const: aspeed,ast2600-i2cv2
>>> +
>>> +then:
>>> +  properties:
>>> +    reg:
>>> +      minItems: 2
>>> +  required:
>>> +    - aspeed,global-regs
>>
>> else:
>>   aspeed,global-regs: false
>> and the same for other v2 properties
>>
> 
> Does modify by following? 
> 
> allOf:
>  -if:
>    properties:
>     compatible:
>       contains:
>         const: aspeed,ast2600-i2cv2
> 
>  then:
>   properties:
>     reg:
>       minItems: 2
>   required:
>     - aspeed,global-regs
>  else:
>     - aspeed,global-regs: false
>     -aspeed,timeout: false
>     - aspeed,xfer-mode: false

yes



Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-03  8:20       ` Krzysztof Kozlowski
@ 2023-03-03  8:28         ` Ryan Chen
  2023-03-03  8:50           ` Krzysztof Kozlowski
  2023-03-12 12:33         ` Andi Shyti
  1 sibling, 1 reply; 28+ messages in thread
From: Ryan Chen @ 2023-03-03  8:28 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Wolfram Sang
  Cc: Joel Stanley, Brendan Higgins, Krzysztof Kozlowski,
	Andrew Jeffery, devicetree, Philipp Zabel, Rob Herring,
	Benjamin Herrenschmidt, linux-aspeed, linux-arm-kernel,
	linux-kernel, openbmc, linux-i2c

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Friday, March 3, 2023 4:20 PM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
> <wsa@kernel.org>
> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery <andrew@aj.id.au>;
> devicetree@vger.kernel.org; Philipp Zabel <p.zabel@pengutronix.de>; Rob
> Herring <robh+dt@kernel.org>; Benjamin Herrenschmidt
> <benh@kernel.crashing.org>; linux-aspeed@lists.ozlabs.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> openbmc@lists.ozlabs.org; linux-i2c@vger.kernel.org
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> On 01/03/2023 06:57, Ryan Chen wrote:
> > Hello Krzysztof,
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Sent: Monday, February 27, 2023 4:25 PM
> >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Andrew Jeffery
> >> <andrew@aj.id.au>; Brendan Higgins <brendan.higgins@linux.dev>;
> >> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Joel Stanley
> >> <joel@jms.id.au>; Rob Herring <robh+dt@kernel.org>; Krzysztof
> >> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Philipp Zabel
> >> <p.zabel@pengutronix.de>; linux-i2c@vger.kernel.org;
> >> openbmc@lists.ozlabs.org; devicetree@vger.kernel.org;
> >> linux-arm-kernel@lists.infradead.org;
> >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 26/02/2023 04:13, Ryan Chen wrote:
> >>> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout
> >>> aspeed,xfer-mode description for ast2600-i2cv2.
> >>>
> >>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> >>> ---
> >>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
> +++++++++++++++++++
> >>>  1 file changed, 44 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> index f597f73ccd87..75de3ce41cf5 100644
> >>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> @@ -49,6 +49,25 @@ properties:
> >>>      description:
> >>>        states that there is another master active on this bus
> >>>
> >>> +  aspeed,timeout:
> >>> +    type: boolean
> >>> +    description: I2C bus timeout enable for master/slave mode
> >>
> >> Nothing improved here in regards to my last comment.
> >
> > Yes, as I know your require is about " DT binding to represent hardware
> setup"
> > So I add more description about aspeed,timeout as blow.
> >
> > ASPEED SOC chip is server product, i2c bus may have fingerprint connect to
> another board. And also support hotplug.
> > The following is board-specific design example.
> > Board A                                         Board B
> > -------------------------                       ------------------------
> > |i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
> > |i2c bus#2(master)-> tmp i2c device |          |
> |
> > |i2c bus#3(master)-> adc i2c device |          |
> |
> > -------------------------                       ------------------------
> >
> > aspeed,timout properites:
> > For example I2C controller as slave mode, and suddenly disconnected.
> > Slave state machine will keep waiting for master clock in for rx/tx transmit.
> > So it need timeout setting to enable timeout unlock controller state.
> > And in another side. In Master side also need avoid suddenly slave
> miss(un-plug), Master will timeout and release the SDA/SCL.
> >
> > Do you mean add those description into ore aspeed,timout properites
> description?
> 
> You are describing here one particular feature you want to enable in the driver
> which looks non-scalable and more difficult to configure/use.
> What I was looking for is to describe the actual configuration you have (e.g.
> multi-master) which leads to enable or disable such feature in your hardware.
> Especially that bool value does not scale later to actual timeout values in time
> (ms)...
> 
> I don't know I2C that much, but I wonder - why this should be specific to
> Aspeed I2C and no other I2C controllers implement it? IOW, this looks quite
> generic and every I2C controller should have it. Adding it specific to Aspeed
> suggests that either we miss a generic property or this should not be in DT at
> all (because no one else has it...).
> 
> Also I wonder, why you wouldn't enable timeout always...
> 
> +Cc Wolfram,
> Maybe you know whether bool "timeout" property for one controller makes
> sense? Why we do not have it for all controllers?
> 
Because, i2c bus didn’t specific timeout.
But SMBus defines a clock low time-out, TIMEOUT of 35 ms. 

It have definition in SMBus specification. 
http://smbus.org/specs/SMBus_3_1_20180319.pdf
You can check Page 18, Note3 that have timeout description.

> >>
> >>> +
> >>> +  aspeed,xfer-mode:
> >>> +    description: |
> >>> +      I2C bus transfer mode selection.
> >>> +      - "byte": I2C bus byte transfer mode.
> >>> +      - "buffered": I2C bus buffer register transfer mode.
> >>> +      - "dma": I2C bus dma transfer mode (default)
> >>> +    items:
> >>> +      enum: [byte, buffered, dma]
> >>> +    maxItems: 1
> >>
> >> Drop, not an array.
> >>
> >>> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> >>
> >> Wrong ref. This is not an array, but one string.
> >
> > Sorry, I can't catch your "one string" point.
> 
> How many strings you are going to have in this property? If one
> (maxItems: 1), then this is not an array.
> 
> > Could you point me what ref I can refer to?
> > That I can check into Linux example. Thanks a lot.
> >>
> >>> +
> >>> +  aspeed,global-regs:
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>> +    description: The phandle of i2c global register node.
> >>> +
> >>>  required:
> >>>    - reg
> >>>    - compatible
> >>> @@ -57,6 +76,19 @@ required:
> >>>
> >>>  unevaluatedProperties: false
> >>>
> >>> +if:
> >>
> >> This should be under allOf (in this location)
> >>
> >>> +  properties:
> >>> +    compatible:
> >>> +      contains:
> >>> +        const: aspeed,ast2600-i2cv2
> >>> +
> >>> +then:
> >>> +  properties:
> >>> +    reg:
> >>> +      minItems: 2
> >>> +  required:
> >>> +    - aspeed,global-regs
> >>
> >> else:
> >>   aspeed,global-regs: false
> >> and the same for other v2 properties
> >>
> >
> > Does modify by following?
> >
> > allOf:
> >  -if:
> >    properties:
> >     compatible:
> >       contains:
> >         const: aspeed,ast2600-i2cv2
> >
> >  then:
> >   properties:
> >     reg:
> >       minItems: 2
> >   required:
> >     - aspeed,global-regs
> >  else:
> >     - aspeed,global-regs: false
> >     -aspeed,timeout: false
> >     - aspeed,xfer-mode: false
> 
> yes
> 
> 
> 
> Best regards,
> Krzysztof


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-03  8:28         ` Ryan Chen
@ 2023-03-03  8:50           ` Krzysztof Kozlowski
  2023-03-03  8:55             ` Ryan Chen
  0 siblings, 1 reply; 28+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-03  8:50 UTC (permalink / raw)
  To: Ryan Chen, Wolfram Sang
  Cc: Joel Stanley, Brendan Higgins, Krzysztof Kozlowski,
	Andrew Jeffery, devicetree, Philipp Zabel, Rob Herring,
	Benjamin Herrenschmidt, linux-aspeed, linux-arm-kernel,
	linux-kernel, openbmc, linux-i2c

On 03/03/2023 09:28, Ryan Chen wrote:
> Hello Krzysztof,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Friday, March 3, 2023 4:20 PM
>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
>> <wsa@kernel.org>
>> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
>> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery <andrew@aj.id.au>;
>> devicetree@vger.kernel.org; Philipp Zabel <p.zabel@pengutronix.de>; Rob
>> Herring <robh+dt@kernel.org>; Benjamin Herrenschmidt
>> <benh@kernel.crashing.org>; linux-aspeed@lists.ozlabs.org;
>> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
>> openbmc@lists.ozlabs.org; linux-i2c@vger.kernel.org
>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
>>
>> On 01/03/2023 06:57, Ryan Chen wrote:
>>> Hello Krzysztof,
>>>
>>>> -----Original Message-----
>>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>> Sent: Monday, February 27, 2023 4:25 PM
>>>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Andrew Jeffery
>>>> <andrew@aj.id.au>; Brendan Higgins <brendan.higgins@linux.dev>;
>>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Joel Stanley
>>>> <joel@jms.id.au>; Rob Herring <robh+dt@kernel.org>; Krzysztof
>>>> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Philipp Zabel
>>>> <p.zabel@pengutronix.de>; linux-i2c@vger.kernel.org;
>>>> openbmc@lists.ozlabs.org; devicetree@vger.kernel.org;
>>>> linux-arm-kernel@lists.infradead.org;
>>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org
>>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
>>>> AST2600-i2cv2
>>>>
>>>> On 26/02/2023 04:13, Ryan Chen wrote:
>>>>> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout
>>>>> aspeed,xfer-mode description for ast2600-i2cv2.
>>>>>
>>>>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
>>>>> ---
>>>>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
>> +++++++++++++++++++
>>>>>  1 file changed, 44 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>>>> b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>>>> index f597f73ccd87..75de3ce41cf5 100644
>>>>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>>>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>>>> @@ -49,6 +49,25 @@ properties:
>>>>>      description:
>>>>>        states that there is another master active on this bus
>>>>>
>>>>> +  aspeed,timeout:
>>>>> +    type: boolean
>>>>> +    description: I2C bus timeout enable for master/slave mode
>>>>
>>>> Nothing improved here in regards to my last comment.
>>>
>>> Yes, as I know your require is about " DT binding to represent hardware
>> setup"
>>> So I add more description about aspeed,timeout as blow.
>>>
>>> ASPEED SOC chip is server product, i2c bus may have fingerprint connect to
>> another board. And also support hotplug.
>>> The following is board-specific design example.
>>> Board A                                         Board B
>>> -------------------------                       ------------------------
>>> |i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
>>> |i2c bus#2(master)-> tmp i2c device |          |
>> |
>>> |i2c bus#3(master)-> adc i2c device |          |
>> |
>>> -------------------------                       ------------------------
>>>
>>> aspeed,timout properites:
>>> For example I2C controller as slave mode, and suddenly disconnected.
>>> Slave state machine will keep waiting for master clock in for rx/tx transmit.
>>> So it need timeout setting to enable timeout unlock controller state.
>>> And in another side. In Master side also need avoid suddenly slave
>> miss(un-plug), Master will timeout and release the SDA/SCL.
>>>
>>> Do you mean add those description into ore aspeed,timout properites
>> description?
>>
>> You are describing here one particular feature you want to enable in the driver
>> which looks non-scalable and more difficult to configure/use.
>> What I was looking for is to describe the actual configuration you have (e.g.
>> multi-master) which leads to enable or disable such feature in your hardware.
>> Especially that bool value does not scale later to actual timeout values in time
>> (ms)...
>>
>> I don't know I2C that much, but I wonder - why this should be specific to
>> Aspeed I2C and no other I2C controllers implement it? IOW, this looks quite
>> generic and every I2C controller should have it. Adding it specific to Aspeed
>> suggests that either we miss a generic property or this should not be in DT at
>> all (because no one else has it...).
>>
>> Also I wonder, why you wouldn't enable timeout always...
>>
>> +Cc Wolfram,
>> Maybe you know whether bool "timeout" property for one controller makes
>> sense? Why we do not have it for all controllers?
>>
> Because, i2c bus didn’t specific timeout.
> But SMBus defines a clock low time-out, TIMEOUT of 35 ms. 
> 
> It have definition in SMBus specification. 
> http://smbus.org/specs/SMBus_3_1_20180319.pdf
> You can check Page 18, Note3 that have timeout description.

Then you have already property for this - "smbus"?

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-03  8:50           ` Krzysztof Kozlowski
@ 2023-03-03  8:55             ` Ryan Chen
  2023-03-03  9:26               ` Krzysztof Kozlowski
  0 siblings, 1 reply; 28+ messages in thread
From: Ryan Chen @ 2023-03-03  8:55 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Wolfram Sang
  Cc: Joel Stanley, Brendan Higgins, Krzysztof Kozlowski,
	Andrew Jeffery, devicetree, Philipp Zabel, Rob Herring,
	Benjamin Herrenschmidt, linux-aspeed, linux-arm-kernel,
	linux-kernel, openbmc, linux-i2c

Hello Krzysztof,
	

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Friday, March 3, 2023 4:51 PM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
> <wsa@kernel.org>
> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery <andrew@aj.id.au>;
> devicetree@vger.kernel.org; Philipp Zabel <p.zabel@pengutronix.de>; Rob
> Herring <robh+dt@kernel.org>; Benjamin Herrenschmidt
> <benh@kernel.crashing.org>; linux-aspeed@lists.ozlabs.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> openbmc@lists.ozlabs.org; linux-i2c@vger.kernel.org
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> On 03/03/2023 09:28, Ryan Chen wrote:
> > Hello Krzysztof,
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Sent: Friday, March 3, 2023 4:20 PM
> >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
> >> <wsa@kernel.org>
> >> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
> >> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery
> >> <andrew@aj.id.au>; devicetree@vger.kernel.org; Philipp Zabel
> >> <p.zabel@pengutronix.de>; Rob Herring <robh+dt@kernel.org>; Benjamin
> >> Herrenschmidt <benh@kernel.crashing.org>;
> >> linux-aspeed@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org;
> >> linux-kernel@vger.kernel.org; openbmc@lists.ozlabs.org;
> >> linux-i2c@vger.kernel.org
> >> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 01/03/2023 06:57, Ryan Chen wrote:
> >>> Hello Krzysztof,
> >>>
> >>>> -----Original Message-----
> >>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >>>> Sent: Monday, February 27, 2023 4:25 PM
> >>>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Andrew Jeffery
> >>>> <andrew@aj.id.au>; Brendan Higgins <brendan.higgins@linux.dev>;
> >>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Joel Stanley
> >>>> <joel@jms.id.au>; Rob Herring <robh+dt@kernel.org>; Krzysztof
> >>>> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Philipp Zabel
> >>>> <p.zabel@pengutronix.de>; linux-i2c@vger.kernel.org;
> >>>> openbmc@lists.ozlabs.org; devicetree@vger.kernel.org;
> >>>> linux-arm-kernel@lists.infradead.org;
> >>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org
> >>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >>>> AST2600-i2cv2
> >>>>
> >>>> On 26/02/2023 04:13, Ryan Chen wrote:
> >>>>> Add ast2600-i2cv2 compatible and aspeed,global-regs,
> >>>>> aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.
> >>>>>
> >>>>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> >>>>> ---
> >>>>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
> >> +++++++++++++++++++
> >>>>>  1 file changed, 44 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>>>> b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>>>> index f597f73ccd87..75de3ce41cf5 100644
> >>>>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>>>> @@ -49,6 +49,25 @@ properties:
> >>>>>      description:
> >>>>>        states that there is another master active on this bus
> >>>>>
> >>>>> +  aspeed,timeout:
> >>>>> +    type: boolean
> >>>>> +    description: I2C bus timeout enable for master/slave mode
> >>>>
> >>>> Nothing improved here in regards to my last comment.
> >>>
> >>> Yes, as I know your require is about " DT binding to represent
> >>> hardware
> >> setup"
> >>> So I add more description about aspeed,timeout as blow.
> >>>
> >>> ASPEED SOC chip is server product, i2c bus may have fingerprint
> >>> connect to
> >> another board. And also support hotplug.
> >>> The following is board-specific design example.
> >>> Board A                                         Board B
> >>> -------------------------                       ------------------------
> >>> |i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
> >>> |i2c bus#2(master)-> tmp i2c device |          |
> >> |
> >>> |i2c bus#3(master)-> adc i2c device |          |
> >> |
> >>> -------------------------                       ------------------------
> >>>
> >>> aspeed,timout properites:
> >>> For example I2C controller as slave mode, and suddenly disconnected.
> >>> Slave state machine will keep waiting for master clock in for rx/tx
> transmit.
> >>> So it need timeout setting to enable timeout unlock controller state.
> >>> And in another side. In Master side also need avoid suddenly slave
> >> miss(un-plug), Master will timeout and release the SDA/SCL.
> >>>
> >>> Do you mean add those description into ore aspeed,timout properites
> >> description?
> >>
> >> You are describing here one particular feature you want to enable in
> >> the driver which looks non-scalable and more difficult to configure/use.
> >> What I was looking for is to describe the actual configuration you have (e.g.
> >> multi-master) which leads to enable or disable such feature in your
> hardware.
> >> Especially that bool value does not scale later to actual timeout
> >> values in time (ms)...
> >>
> >> I don't know I2C that much, but I wonder - why this should be
> >> specific to Aspeed I2C and no other I2C controllers implement it?
> >> IOW, this looks quite generic and every I2C controller should have
> >> it. Adding it specific to Aspeed suggests that either we miss a
> >> generic property or this should not be in DT at all (because no one else has
> it...).
> >>
> >> Also I wonder, why you wouldn't enable timeout always...
> >>
> >> +Cc Wolfram,
> >> Maybe you know whether bool "timeout" property for one controller
> >> makes sense? Why we do not have it for all controllers?
> >>
> > Because, i2c bus didn’t specific timeout.
> > But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
> >
> > It have definition in SMBus specification.
> > http://smbus.org/specs/SMBus_3_1_20180319.pdf
> > You can check Page 18, Note3 that have timeout description.
> 
> Then you have already property for this - "smbus"?
To be a property "smbus", that would be a big topic, 
I saw fsl i2c also have this.
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml#L43-L47
So, I just think the "timeout" property.

 
Best regards,
Ryan Chen

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-03  8:55             ` Ryan Chen
@ 2023-03-03  9:26               ` Krzysztof Kozlowski
  2023-03-03 10:16                 ` Ryan Chen
  0 siblings, 1 reply; 28+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-03  9:26 UTC (permalink / raw)
  To: Ryan Chen, Wolfram Sang
  Cc: Joel Stanley, Brendan Higgins, Krzysztof Kozlowski,
	Andrew Jeffery, devicetree, Philipp Zabel, Rob Herring,
	Benjamin Herrenschmidt, linux-aspeed, linux-arm-kernel,
	linux-kernel, openbmc, linux-i2c

On 03/03/2023 09:55, Ryan Chen wrote:
> Hello Krzysztof,
> 	
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Friday, March 3, 2023 4:51 PM
>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
>> <wsa@kernel.org>
>> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
>> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery <andrew@aj.id.au>;
>> devicetree@vger.kernel.org; Philipp Zabel <p.zabel@pengutronix.de>; Rob
>> Herring <robh+dt@kernel.org>; Benjamin Herrenschmidt
>> <benh@kernel.crashing.org>; linux-aspeed@lists.ozlabs.org;
>> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
>> openbmc@lists.ozlabs.org; linux-i2c@vger.kernel.org
>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
>>
>> On 03/03/2023 09:28, Ryan Chen wrote:
>>> Hello Krzysztof,
>>>
>>>> -----Original Message-----
>>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>> Sent: Friday, March 3, 2023 4:20 PM
>>>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
>>>> <wsa@kernel.org>
>>>> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
>>>> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
>>>> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery
>>>> <andrew@aj.id.au>; devicetree@vger.kernel.org; Philipp Zabel
>>>> <p.zabel@pengutronix.de>; Rob Herring <robh+dt@kernel.org>; Benjamin
>>>> Herrenschmidt <benh@kernel.crashing.org>;
>>>> linux-aspeed@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org;
>>>> linux-kernel@vger.kernel.org; openbmc@lists.ozlabs.org;
>>>> linux-i2c@vger.kernel.org
>>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
>>>> AST2600-i2cv2
>>>>
>>>> On 01/03/2023 06:57, Ryan Chen wrote:
>>>>> Hello Krzysztof,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>>> Sent: Monday, February 27, 2023 4:25 PM
>>>>>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Andrew Jeffery
>>>>>> <andrew@aj.id.au>; Brendan Higgins <brendan.higgins@linux.dev>;
>>>>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Joel Stanley
>>>>>> <joel@jms.id.au>; Rob Herring <robh+dt@kernel.org>; Krzysztof
>>>>>> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Philipp Zabel
>>>>>> <p.zabel@pengutronix.de>; linux-i2c@vger.kernel.org;
>>>>>> openbmc@lists.ozlabs.org; devicetree@vger.kernel.org;
>>>>>> linux-arm-kernel@lists.infradead.org;
>>>>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org
>>>>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
>>>>>> AST2600-i2cv2
>>>>>>
>>>>>> On 26/02/2023 04:13, Ryan Chen wrote:
>>>>>>> Add ast2600-i2cv2 compatible and aspeed,global-regs,
>>>>>>> aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.
>>>>>>>
>>>>>>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
>>>> +++++++++++++++++++
>>>>>>>  1 file changed, 44 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>>>>>> b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>>>>>> index f597f73ccd87..75de3ce41cf5 100644
>>>>>>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>>>>>> @@ -49,6 +49,25 @@ properties:
>>>>>>>      description:
>>>>>>>        states that there is another master active on this bus
>>>>>>>
>>>>>>> +  aspeed,timeout:
>>>>>>> +    type: boolean
>>>>>>> +    description: I2C bus timeout enable for master/slave mode
>>>>>>
>>>>>> Nothing improved here in regards to my last comment.
>>>>>
>>>>> Yes, as I know your require is about " DT binding to represent
>>>>> hardware
>>>> setup"
>>>>> So I add more description about aspeed,timeout as blow.
>>>>>
>>>>> ASPEED SOC chip is server product, i2c bus may have fingerprint
>>>>> connect to
>>>> another board. And also support hotplug.
>>>>> The following is board-specific design example.
>>>>> Board A                                         Board B
>>>>> -------------------------                       ------------------------
>>>>> |i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
>>>>> |i2c bus#2(master)-> tmp i2c device |          |
>>>> |
>>>>> |i2c bus#3(master)-> adc i2c device |          |
>>>> |
>>>>> -------------------------                       ------------------------
>>>>>
>>>>> aspeed,timout properites:
>>>>> For example I2C controller as slave mode, and suddenly disconnected.
>>>>> Slave state machine will keep waiting for master clock in for rx/tx
>> transmit.
>>>>> So it need timeout setting to enable timeout unlock controller state.
>>>>> And in another side. In Master side also need avoid suddenly slave
>>>> miss(un-plug), Master will timeout and release the SDA/SCL.
>>>>>
>>>>> Do you mean add those description into ore aspeed,timout properites
>>>> description?
>>>>
>>>> You are describing here one particular feature you want to enable in
>>>> the driver which looks non-scalable and more difficult to configure/use.
>>>> What I was looking for is to describe the actual configuration you have (e.g.
>>>> multi-master) which leads to enable or disable such feature in your
>> hardware.
>>>> Especially that bool value does not scale later to actual timeout
>>>> values in time (ms)...
>>>>
>>>> I don't know I2C that much, but I wonder - why this should be
>>>> specific to Aspeed I2C and no other I2C controllers implement it?
>>>> IOW, this looks quite generic and every I2C controller should have
>>>> it. Adding it specific to Aspeed suggests that either we miss a
>>>> generic property or this should not be in DT at all (because no one else has
>> it...).
>>>>
>>>> Also I wonder, why you wouldn't enable timeout always...
>>>>
>>>> +Cc Wolfram,
>>>> Maybe you know whether bool "timeout" property for one controller
>>>> makes sense? Why we do not have it for all controllers?
>>>>
>>> Because, i2c bus didn’t specific timeout.
>>> But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
>>>
>>> It have definition in SMBus specification.
>>> http://smbus.org/specs/SMBus_3_1_20180319.pdf
>>> You can check Page 18, Note3 that have timeout description.
>>
>> Then you have already property for this - "smbus"?
> To be a property "smbus", that would be a big topic, 
> I saw fsl i2c also have this.
> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml#L43-L47
> So, I just think the "timeout" property.

Yeah and this is the only place. It also differs because it allows
actual timeout values.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-03  9:26               ` Krzysztof Kozlowski
@ 2023-03-03 10:16                 ` Ryan Chen
  2023-03-03 10:41                   ` Krzysztof Kozlowski
  0 siblings, 1 reply; 28+ messages in thread
From: Ryan Chen @ 2023-03-03 10:16 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Wolfram Sang
  Cc: Joel Stanley, Brendan Higgins, Krzysztof Kozlowski,
	Andrew Jeffery, devicetree, Philipp Zabel, Rob Herring,
	Benjamin Herrenschmidt, linux-aspeed, linux-arm-kernel,
	linux-kernel, openbmc, linux-i2c

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Friday, March 3, 2023 5:26 PM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
> <wsa@kernel.org>
> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery <andrew@aj.id.au>;
> devicetree@vger.kernel.org; Philipp Zabel <p.zabel@pengutronix.de>; Rob
> Herring <robh+dt@kernel.org>; Benjamin Herrenschmidt
> <benh@kernel.crashing.org>; linux-aspeed@lists.ozlabs.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> openbmc@lists.ozlabs.org; linux-i2c@vger.kernel.org
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 03/03/2023 09:55, Ryan Chen wrote:
> > Hello Krzysztof,
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Sent: Friday, March 3, 2023 4:51 PM
> >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
> >> <wsa@kernel.org>
> >> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
> >> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery
> >> <andrew@aj.id.au>; devicetree@vger.kernel.org; Philipp Zabel
> >> <p.zabel@pengutronix.de>; Rob Herring <robh+dt@kernel.org>;
> Benjamin
> >> Herrenschmidt <benh@kernel.crashing.org>;
> >> linux-aspeed@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org;
> >> linux-kernel@vger.kernel.org; openbmc@lists.ozlabs.org;
> >> linux-i2c@vger.kernel.org
> >> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 03/03/2023 09:28, Ryan Chen wrote:
> >>> Hello Krzysztof,
> >>>
> >>>> -----Original Message-----
> >>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >>>> Sent: Friday, March 3, 2023 4:20 PM
> >>>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
> >>>> <wsa@kernel.org>
> >>>> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
> >>>> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
> >>>> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery
> >>>> <andrew@aj.id.au>; devicetree@vger.kernel.org; Philipp Zabel
> >>>> <p.zabel@pengutronix.de>; Rob Herring <robh+dt@kernel.org>;
> >>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>;
> >>>> linux-aspeed@lists.ozlabs.org;
> >>>> linux-arm-kernel@lists.infradead.org;
> >>>> linux-kernel@vger.kernel.org; openbmc@lists.ozlabs.org;
> >>>> linux-i2c@vger.kernel.org
> >>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >>>> AST2600-i2cv2
> >>>>
> >>>> On 01/03/2023 06:57, Ryan Chen wrote:
> >>>>> Hello Krzysztof,
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >>>>>> Sent: Monday, February 27, 2023 4:25 PM
> >>>>>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Andrew Jeffery
> >>>>>> <andrew@aj.id.au>; Brendan Higgins <brendan.higgins@linux.dev>;
> >>>>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Joel Stanley
> >>>>>> <joel@jms.id.au>; Rob Herring <robh+dt@kernel.org>; Krzysztof
> >>>>>> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Philipp Zabel
> >>>>>> <p.zabel@pengutronix.de>; linux-i2c@vger.kernel.org;
> >>>>>> openbmc@lists.ozlabs.org; devicetree@vger.kernel.org;
> >>>>>> linux-arm-kernel@lists.infradead.org;
> >>>>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org
> >>>>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >>>>>> AST2600-i2cv2
> >>>>>>
> >>>>>> On 26/02/2023 04:13, Ryan Chen wrote:
> >>>>>>> Add ast2600-i2cv2 compatible and aspeed,global-regs,
> >>>>>>> aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.
> >>>>>>>
> >>>>>>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> >>>>>>> ---
> >>>>>>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
> >>>> +++++++++++++++++++
> >>>>>>>  1 file changed, 44 insertions(+)
> >>>>>>>
> >>>>>>> diff --git
> >>>>>>> a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>>>>>> b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>>>>>> index f597f73ccd87..75de3ce41cf5 100644
> >>>>>>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>>>>>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>>>>>> @@ -49,6 +49,25 @@ properties:
> >>>>>>>      description:
> >>>>>>>        states that there is another master active on this bus
> >>>>>>>
> >>>>>>> +  aspeed,timeout:
> >>>>>>> +    type: boolean
> >>>>>>> +    description: I2C bus timeout enable for master/slave mode
> >>>>>>
> >>>>>> Nothing improved here in regards to my last comment.
> >>>>>
> >>>>> Yes, as I know your require is about " DT binding to represent
> >>>>> hardware
> >>>> setup"
> >>>>> So I add more description about aspeed,timeout as blow.
> >>>>>
> >>>>> ASPEED SOC chip is server product, i2c bus may have fingerprint
> >>>>> connect to
> >>>> another board. And also support hotplug.
> >>>>> The following is board-specific design example.
> >>>>> Board A                                         Board B
> >>>>> -------------------------                       ------------------------
> >>>>> |i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x
> (master/slave)|
> >>>>> |i2c bus#2(master)-> tmp i2c device |          |
> >>>> |
> >>>>> |i2c bus#3(master)-> adc i2c device |          |
> >>>> |
> >>>>> -------------------------                       ------------------------
> >>>>>
> >>>>> aspeed,timout properites:
> >>>>> For example I2C controller as slave mode, and suddenly
> disconnected.
> >>>>> Slave state machine will keep waiting for master clock in for
> >>>>> rx/tx
> >> transmit.
> >>>>> So it need timeout setting to enable timeout unlock controller state.
> >>>>> And in another side. In Master side also need avoid suddenly slave
> >>>> miss(un-plug), Master will timeout and release the SDA/SCL.
> >>>>>
> >>>>> Do you mean add those description into ore aspeed,timout
> >>>>> properites
> >>>> description?
> >>>>
> >>>> You are describing here one particular feature you want to enable
> >>>> in the driver which looks non-scalable and more difficult to
> configure/use.
> >>>> What I was looking for is to describe the actual configuration you have
> (e.g.
> >>>> multi-master) which leads to enable or disable such feature in your
> >> hardware.
> >>>> Especially that bool value does not scale later to actual timeout
> >>>> values in time (ms)...
> >>>>
> >>>> I don't know I2C that much, but I wonder - why this should be
> >>>> specific to Aspeed I2C and no other I2C controllers implement it?
> >>>> IOW, this looks quite generic and every I2C controller should have
> >>>> it. Adding it specific to Aspeed suggests that either we miss a
> >>>> generic property or this should not be in DT at all (because no one
> >>>> else has
> >> it...).
> >>>>
> >>>> Also I wonder, why you wouldn't enable timeout always...
> >>>>
> >>>> +Cc Wolfram,
> >>>> Maybe you know whether bool "timeout" property for one controller
> >>>> makes sense? Why we do not have it for all controllers?
> >>>>
> >>> Because, i2c bus didn’t specific timeout.
> >>> But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
> >>>
> >>> It have definition in SMBus specification.
> >>> http://smbus.org/specs/SMBus_3_1_20180319.pdf
> >>> You can check Page 18, Note3 that have timeout description.
> >>
> >> Then you have already property for this - "smbus"?
> > To be a property "smbus", that would be a big topic, I saw fsl i2c
> > also have this.
> > https://github.com/torvalds/linux/blob/master/Documentation/devicetree
> > /bindings/i2c/i2c-mpc.yaml#L43-L47
> > So, I just think the "timeout" property.
> 
> Yeah and this is the only place. It also differs because it allows actual
> timeout values.
Thanks, So can I still keep the property "aspeed,timeout" here?
It is the only place. 

Best regards,
Ryan Chen


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-03 10:16                 ` Ryan Chen
@ 2023-03-03 10:41                   ` Krzysztof Kozlowski
  2023-03-04  1:33                     ` Ryan Chen
  0 siblings, 1 reply; 28+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-03 10:41 UTC (permalink / raw)
  To: Ryan Chen, Wolfram Sang
  Cc: Joel Stanley, Brendan Higgins, Krzysztof Kozlowski,
	Andrew Jeffery, devicetree, Philipp Zabel, Rob Herring,
	Benjamin Herrenschmidt, linux-aspeed, linux-arm-kernel,
	linux-kernel, openbmc, linux-i2c

On 03/03/2023 11:16, Ryan Chen wrote:
>>>>>>> aspeed,timout properites:
>>>>>>> For example I2C controller as slave mode, and suddenly
>> disconnected.
>>>>>>> Slave state machine will keep waiting for master clock in for
>>>>>>> rx/tx
>>>> transmit.
>>>>>>> So it need timeout setting to enable timeout unlock controller state.
>>>>>>> And in another side. In Master side also need avoid suddenly slave
>>>>>> miss(un-plug), Master will timeout and release the SDA/SCL.
>>>>>>>
>>>>>>> Do you mean add those description into ore aspeed,timout
>>>>>>> properites
>>>>>> description?
>>>>>>
>>>>>> You are describing here one particular feature you want to enable
>>>>>> in the driver which looks non-scalable and more difficult to
>> configure/use.
>>>>>> What I was looking for is to describe the actual configuration you have
>> (e.g.
>>>>>> multi-master) which leads to enable or disable such feature in your
>>>> hardware.
>>>>>> Especially that bool value does not scale later to actual timeout
>>>>>> values in time (ms)...
>>>>>>
>>>>>> I don't know I2C that much, but I wonder - why this should be
>>>>>> specific to Aspeed I2C and no other I2C controllers implement it?
>>>>>> IOW, this looks quite generic and every I2C controller should have
>>>>>> it. Adding it specific to Aspeed suggests that either we miss a
>>>>>> generic property or this should not be in DT at all (because no one
>>>>>> else has
>>>> it...).
>>>>>>
>>>>>> Also I wonder, why you wouldn't enable timeout always...
>>>>>>
>>>>>> +Cc Wolfram,
>>>>>> Maybe you know whether bool "timeout" property for one controller
>>>>>> makes sense? Why we do not have it for all controllers?
>>>>>>
>>>>> Because, i2c bus didn’t specific timeout.
>>>>> But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
>>>>>
>>>>> It have definition in SMBus specification.
>>>>> http://smbus.org/specs/SMBus_3_1_20180319.pdf
>>>>> You can check Page 18, Note3 that have timeout description.
>>>>
>>>> Then you have already property for this - "smbus"?
>>> To be a property "smbus", that would be a big topic, I saw fsl i2c
>>> also have this.
>>> https://github.com/torvalds/linux/blob/master/Documentation/devicetree
>>> /bindings/i2c/i2c-mpc.yaml#L43-L47
>>> So, I just think the "timeout" property.
>>
>> Yeah and this is the only place. It also differs because it allows actual
>> timeout values.
> Thanks, So can I still keep the property "aspeed,timeout" here?
> It is the only place. 

No, because none of my concerns above are addressed.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-03 10:41                   ` Krzysztof Kozlowski
@ 2023-03-04  1:33                     ` Ryan Chen
  2023-03-05  9:49                       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 28+ messages in thread
From: Ryan Chen @ 2023-03-04  1:33 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Wolfram Sang
  Cc: Joel Stanley, Brendan Higgins, Krzysztof Kozlowski,
	Andrew Jeffery, devicetree, Philipp Zabel, Rob Herring,
	Benjamin Herrenschmidt, linux-aspeed, linux-arm-kernel,
	linux-kernel, openbmc, linux-i2c

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Friday, March 3, 2023 6:41 PM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
> <wsa@kernel.org>
> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery <andrew@aj.id.au>;
> devicetree@vger.kernel.org; Philipp Zabel <p.zabel@pengutronix.de>; Rob
> Herring <robh+dt@kernel.org>; Benjamin Herrenschmidt
> <benh@kernel.crashing.org>; linux-aspeed@lists.ozlabs.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> openbmc@lists.ozlabs.org; linux-i2c@vger.kernel.org
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> On 03/03/2023 11:16, Ryan Chen wrote:
> >>>>>>> aspeed,timout properites:
> >>>>>>> For example I2C controller as slave mode, and suddenly
> >> disconnected.
> >>>>>>> Slave state machine will keep waiting for master clock in for
> >>>>>>> rx/tx
> >>>> transmit.
> >>>>>>> So it need timeout setting to enable timeout unlock controller state.
> >>>>>>> And in another side. In Master side also need avoid suddenly
> >>>>>>> slave
> >>>>>> miss(un-plug), Master will timeout and release the SDA/SCL.
> >>>>>>>
> >>>>>>> Do you mean add those description into ore aspeed,timout
> >>>>>>> properites
> >>>>>> description?
> >>>>>>
> >>>>>> You are describing here one particular feature you want to enable
> >>>>>> in the driver which looks non-scalable and more difficult to
> >> configure/use.
> >>>>>> What I was looking for is to describe the actual configuration
> >>>>>> you have
> >> (e.g.
> >>>>>> multi-master) which leads to enable or disable such feature in
> >>>>>> your
> >>>> hardware.
> >>>>>> Especially that bool value does not scale later to actual timeout
> >>>>>> values in time (ms)...
> >>>>>>
> >>>>>> I don't know I2C that much, but I wonder - why this should be
> >>>>>> specific to Aspeed I2C and no other I2C controllers implement it?
> >>>>>> IOW, this looks quite generic and every I2C controller should
> >>>>>> have it. Adding it specific to Aspeed suggests that either we
> >>>>>> miss a generic property or this should not be in DT at all
> >>>>>> (because no one else has
> >>>> it...).
> >>>>>>
> >>>>>> Also I wonder, why you wouldn't enable timeout always...
> >>>>>>
> >>>>>> +Cc Wolfram,
> >>>>>> Maybe you know whether bool "timeout" property for one controller
> >>>>>> makes sense? Why we do not have it for all controllers?
> >>>>>>
> >>>>> Because, i2c bus didn’t specific timeout.
> >>>>> But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
> >>>>>
> >>>>> It have definition in SMBus specification.
> >>>>> http://smbus.org/specs/SMBus_3_1_20180319.pdf
> >>>>> You can check Page 18, Note3 that have timeout description.
> >>>>
> >>>> Then you have already property for this - "smbus"?
> >>> To be a property "smbus", that would be a big topic, I saw fsl i2c
> >>> also have this.
> >>> https://github.com/torvalds/linux/blob/master/Documentation/devicetr
> >>> ee
> >>> /bindings/i2c/i2c-mpc.yaml#L43-L47
> >>> So, I just think the "timeout" property.
> >>
> >> Yeah and this is the only place. It also differs because it allows
> >> actual timeout values.
> > Thanks, So can I still keep the property "aspeed,timeout" here?
> > It is the only place.
> 
> No, because none of my concerns above are addressed.
> 
Thanks, I realize your concerns.

So, I modify it like i2c-mpc.yaml 
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml#L43-L47

  aspeed,timeout:
    $ref: /schemas/types.yaml#/definitions/uint32
    description: |
      I2C bus timeout in microseconds
Is this way acceptable? 

Best regards,
Ryan Chen

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-04  1:33                     ` Ryan Chen
@ 2023-03-05  9:49                       ` Krzysztof Kozlowski
  2023-03-06  0:48                         ` Ryan Chen
  0 siblings, 1 reply; 28+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-05  9:49 UTC (permalink / raw)
  To: Ryan Chen, Wolfram Sang
  Cc: Joel Stanley, Brendan Higgins, Krzysztof Kozlowski,
	Andrew Jeffery, devicetree, Philipp Zabel, Rob Herring,
	Benjamin Herrenschmidt, linux-aspeed, linux-arm-kernel,
	linux-kernel, openbmc, linux-i2c

On 04/03/2023 02:33, Ryan Chen wrote:
> Hello Krzysztof,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Friday, March 3, 2023 6:41 PM
>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
>> <wsa@kernel.org>
>> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
>> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery <andrew@aj.id.au>;
>> devicetree@vger.kernel.org; Philipp Zabel <p.zabel@pengutronix.de>; Rob
>> Herring <robh+dt@kernel.org>; Benjamin Herrenschmidt
>> <benh@kernel.crashing.org>; linux-aspeed@lists.ozlabs.org;
>> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
>> openbmc@lists.ozlabs.org; linux-i2c@vger.kernel.org
>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
>>
>> On 03/03/2023 11:16, Ryan Chen wrote:
>>>>>>>>> aspeed,timout properites:
>>>>>>>>> For example I2C controller as slave mode, and suddenly
>>>> disconnected.
>>>>>>>>> Slave state machine will keep waiting for master clock in for
>>>>>>>>> rx/tx
>>>>>> transmit.
>>>>>>>>> So it need timeout setting to enable timeout unlock controller state.
>>>>>>>>> And in another side. In Master side also need avoid suddenly
>>>>>>>>> slave
>>>>>>>> miss(un-plug), Master will timeout and release the SDA/SCL.
>>>>>>>>>
>>>>>>>>> Do you mean add those description into ore aspeed,timout
>>>>>>>>> properites
>>>>>>>> description?
>>>>>>>>
>>>>>>>> You are describing here one particular feature you want to enable
>>>>>>>> in the driver which looks non-scalable and more difficult to
>>>> configure/use.
>>>>>>>> What I was looking for is to describe the actual configuration
>>>>>>>> you have
>>>> (e.g.
>>>>>>>> multi-master) which leads to enable or disable such feature in
>>>>>>>> your
>>>>>> hardware.
>>>>>>>> Especially that bool value does not scale later to actual timeout
>>>>>>>> values in time (ms)...
>>>>>>>>
>>>>>>>> I don't know I2C that much, but I wonder - why this should be
>>>>>>>> specific to Aspeed I2C and no other I2C controllers implement it?
>>>>>>>> IOW, this looks quite generic and every I2C controller should
>>>>>>>> have it. Adding it specific to Aspeed suggests that either we
>>>>>>>> miss a generic property or this should not be in DT at all
>>>>>>>> (because no one else has
>>>>>> it...).
>>>>>>>>
>>>>>>>> Also I wonder, why you wouldn't enable timeout always...
>>>>>>>>
>>>>>>>> +Cc Wolfram,
>>>>>>>> Maybe you know whether bool "timeout" property for one controller
>>>>>>>> makes sense? Why we do not have it for all controllers?
>>>>>>>>
>>>>>>> Because, i2c bus didn’t specific timeout.
>>>>>>> But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
>>>>>>>
>>>>>>> It have definition in SMBus specification.
>>>>>>> http://smbus.org/specs/SMBus_3_1_20180319.pdf
>>>>>>> You can check Page 18, Note3 that have timeout description.
>>>>>>
>>>>>> Then you have already property for this - "smbus"?
>>>>> To be a property "smbus", that would be a big topic, I saw fsl i2c
>>>>> also have this.
>>>>> https://github.com/torvalds/linux/blob/master/Documentation/devicetr
>>>>> ee
>>>>> /bindings/i2c/i2c-mpc.yaml#L43-L47
>>>>> So, I just think the "timeout" property.
>>>>
>>>> Yeah and this is the only place. It also differs because it allows
>>>> actual timeout values.
>>> Thanks, So can I still keep the property "aspeed,timeout" here?
>>> It is the only place.
>>
>> No, because none of my concerns above are addressed.
>>
> Thanks, I realize your concerns.
> 
> So, I modify it like i2c-mpc.yaml 
> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml#L43-L47
> 
>   aspeed,timeout:
>     $ref: /schemas/types.yaml#/definitions/uint32
>     description: |
>       I2C bus timeout in microseconds
> Is this way acceptable? 

So, let's repeat my last questions:

1. Why you wouldn't enable timeout always...

You wrote:
> http://smbus.org/specs/SMBus_3_1_20180319.pdf
> You can check Page 18, Note3 that have timeout description.

which indicates you should always use timeout, doesn't it?

2. Why we do not have it for all controllers with SMBus v3? Why this one
is special?

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-05  9:49                       ` Krzysztof Kozlowski
@ 2023-03-06  0:48                         ` Ryan Chen
  2023-03-07  8:11                           ` Krzysztof Kozlowski
  0 siblings, 1 reply; 28+ messages in thread
From: Ryan Chen @ 2023-03-06  0:48 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Wolfram Sang
  Cc: Joel Stanley, Brendan Higgins, Krzysztof Kozlowski,
	Andrew Jeffery, devicetree, Philipp Zabel, Rob Herring,
	Benjamin Herrenschmidt, linux-aspeed, linux-arm-kernel,
	linux-kernel, openbmc, linux-i2c

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Sunday, March 5, 2023 5:49 PM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
> <wsa@kernel.org>
> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery <andrew@aj.id.au>;
> devicetree@vger.kernel.org; Philipp Zabel <p.zabel@pengutronix.de>; Rob
> Herring <robh+dt@kernel.org>; Benjamin Herrenschmidt
> <benh@kernel.crashing.org>; linux-aspeed@lists.ozlabs.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> openbmc@lists.ozlabs.org; linux-i2c@vger.kernel.org
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> On 04/03/2023 02:33, Ryan Chen wrote:
> > Hello Krzysztof,
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Sent: Friday, March 3, 2023 6:41 PM
> >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
> >> <wsa@kernel.org>
> >> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
> >> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery
> >> <andrew@aj.id.au>; devicetree@vger.kernel.org; Philipp Zabel
> >> <p.zabel@pengutronix.de>; Rob Herring <robh+dt@kernel.org>; Benjamin
> >> Herrenschmidt <benh@kernel.crashing.org>;
> >> linux-aspeed@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org;
> >> linux-kernel@vger.kernel.org; openbmc@lists.ozlabs.org;
> >> linux-i2c@vger.kernel.org
> >> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 03/03/2023 11:16, Ryan Chen wrote:
> >>>>>>>>> aspeed,timout properites:
> >>>>>>>>> For example I2C controller as slave mode, and suddenly
> >>>> disconnected.
> >>>>>>>>> Slave state machine will keep waiting for master clock in for
> >>>>>>>>> rx/tx
> >>>>>> transmit.
> >>>>>>>>> So it need timeout setting to enable timeout unlock controller
> state.
> >>>>>>>>> And in another side. In Master side also need avoid suddenly
> >>>>>>>>> slave
> >>>>>>>> miss(un-plug), Master will timeout and release the SDA/SCL.
> >>>>>>>>>
> >>>>>>>>> Do you mean add those description into ore aspeed,timout
> >>>>>>>>> properites
> >>>>>>>> description?
> >>>>>>>>
> >>>>>>>> You are describing here one particular feature you want to
> >>>>>>>> enable in the driver which looks non-scalable and more
> >>>>>>>> difficult to
> >>>> configure/use.
> >>>>>>>> What I was looking for is to describe the actual configuration
> >>>>>>>> you have
> >>>> (e.g.
> >>>>>>>> multi-master) which leads to enable or disable such feature in
> >>>>>>>> your
> >>>>>> hardware.
> >>>>>>>> Especially that bool value does not scale later to actual
> >>>>>>>> timeout values in time (ms)...
> >>>>>>>>
> >>>>>>>> I don't know I2C that much, but I wonder - why this should be
> >>>>>>>> specific to Aspeed I2C and no other I2C controllers implement it?
> >>>>>>>> IOW, this looks quite generic and every I2C controller should
> >>>>>>>> have it. Adding it specific to Aspeed suggests that either we
> >>>>>>>> miss a generic property or this should not be in DT at all
> >>>>>>>> (because no one else has
> >>>>>> it...).
> >>>>>>>>
> >>>>>>>> Also I wonder, why you wouldn't enable timeout always...
> >>>>>>>>
> >>>>>>>> +Cc Wolfram,
> >>>>>>>> Maybe you know whether bool "timeout" property for one
> >>>>>>>> controller makes sense? Why we do not have it for all controllers?
> >>>>>>>>
> >>>>>>> Because, i2c bus didn’t specific timeout.
> >>>>>>> But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
> >>>>>>>
> >>>>>>> It have definition in SMBus specification.
> >>>>>>> http://smbus.org/specs/SMBus_3_1_20180319.pdf
> >>>>>>> You can check Page 18, Note3 that have timeout description.
> >>>>>>
> >>>>>> Then you have already property for this - "smbus"?
> >>>>> To be a property "smbus", that would be a big topic, I saw fsl i2c
> >>>>> also have this.
> >>>>> https://github.com/torvalds/linux/blob/master/Documentation/device
> >>>>> tr
> >>>>> ee
> >>>>> /bindings/i2c/i2c-mpc.yaml#L43-L47
> >>>>> So, I just think the "timeout" property.
> >>>>
> >>>> Yeah and this is the only place. It also differs because it allows
> >>>> actual timeout values.
> >>> Thanks, So can I still keep the property "aspeed,timeout" here?
> >>> It is the only place.
> >>
> >> No, because none of my concerns above are addressed.
> >>
> > Thanks, I realize your concerns.
> >
> > So, I modify it like i2c-mpc.yaml
> > https://github.com/torvalds/linux/blob/master/Documentation/devicetree
> > /bindings/i2c/i2c-mpc.yaml#L43-L47
> >
> >   aspeed,timeout:
> >     $ref: /schemas/types.yaml#/definitions/uint32
> >     description: |
> >       I2C bus timeout in microseconds
> > Is this way acceptable?
> 
> So, let's repeat my last questions:
> 
> 1. Why you wouldn't enable timeout always...
> 
> You wrote:
> > http://smbus.org/specs/SMBus_3_1_20180319.pdf
> > You can check Page 18, Note3 that have timeout description.
> 
> which indicates you should always use timeout, doesn't it?

Yes, if board design the bus is connected with SMBUS device, it should enable.
But in my previous statement, the board design is two multi-master devices connected each other. 
And both device is transfer with MCTP protocol. 
That will not SMBUS protocol. 
They need have timeout that prevent unexpected un-plug.
I do the study with smbus in Linux, that will different slave call back. Compare with smbus slave and mctp slave.
So in this scenario, that is only enable for timeout. 
 
> 2. Why we do not have it for all controllers with SMBus v3? Why this one is
> special?

Not all bus is connected with smbus. Most are i2c device connected in board.
That will be specific statement for each bus.

Best regards,
Ryan Chen

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-06  0:48                         ` Ryan Chen
@ 2023-03-07  8:11                           ` Krzysztof Kozlowski
  2023-03-07 10:09                             ` Ryan Chen
  0 siblings, 1 reply; 28+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-07  8:11 UTC (permalink / raw)
  To: Ryan Chen, Wolfram Sang
  Cc: Joel Stanley, Brendan Higgins, Krzysztof Kozlowski,
	Andrew Jeffery, devicetree, Philipp Zabel, Rob Herring,
	Benjamin Herrenschmidt, linux-aspeed, linux-arm-kernel,
	linux-kernel, openbmc, linux-i2c

On 06/03/2023 01:48, Ryan Chen wrote:
> Hello Krzysztof,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Sunday, March 5, 2023 5:49 PM
>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
>> <wsa@kernel.org>
>> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
>> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery <andrew@aj.id.au>;
>> devicetree@vger.kernel.org; Philipp Zabel <p.zabel@pengutronix.de>; Rob
>> Herring <robh+dt@kernel.org>; Benjamin Herrenschmidt
>> <benh@kernel.crashing.org>; linux-aspeed@lists.ozlabs.org;
>> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
>> openbmc@lists.ozlabs.org; linux-i2c@vger.kernel.org
>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
>>
>> On 04/03/2023 02:33, Ryan Chen wrote:
>>> Hello Krzysztof,
>>>
>>>> -----Original Message-----
>>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>> Sent: Friday, March 3, 2023 6:41 PM
>>>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
>>>> <wsa@kernel.org>
>>>> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
>>>> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
>>>> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery
>>>> <andrew@aj.id.au>; devicetree@vger.kernel.org; Philipp Zabel
>>>> <p.zabel@pengutronix.de>; Rob Herring <robh+dt@kernel.org>; Benjamin
>>>> Herrenschmidt <benh@kernel.crashing.org>;
>>>> linux-aspeed@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org;
>>>> linux-kernel@vger.kernel.org; openbmc@lists.ozlabs.org;
>>>> linux-i2c@vger.kernel.org
>>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
>>>> AST2600-i2cv2
>>>>
>>>> On 03/03/2023 11:16, Ryan Chen wrote:
>>>>>>>>>>> aspeed,timout properites:
>>>>>>>>>>> For example I2C controller as slave mode, and suddenly
>>>>>> disconnected.
>>>>>>>>>>> Slave state machine will keep waiting for master clock in for
>>>>>>>>>>> rx/tx
>>>>>>>> transmit.
>>>>>>>>>>> So it need timeout setting to enable timeout unlock controller
>> state.
>>>>>>>>>>> And in another side. In Master side also need avoid suddenly
>>>>>>>>>>> slave
>>>>>>>>>> miss(un-plug), Master will timeout and release the SDA/SCL.
>>>>>>>>>>>
>>>>>>>>>>> Do you mean add those description into ore aspeed,timout
>>>>>>>>>>> properites
>>>>>>>>>> description?
>>>>>>>>>>
>>>>>>>>>> You are describing here one particular feature you want to
>>>>>>>>>> enable in the driver which looks non-scalable and more
>>>>>>>>>> difficult to
>>>>>> configure/use.
>>>>>>>>>> What I was looking for is to describe the actual configuration
>>>>>>>>>> you have
>>>>>> (e.g.
>>>>>>>>>> multi-master) which leads to enable or disable such feature in
>>>>>>>>>> your
>>>>>>>> hardware.
>>>>>>>>>> Especially that bool value does not scale later to actual
>>>>>>>>>> timeout values in time (ms)...
>>>>>>>>>>
>>>>>>>>>> I don't know I2C that much, but I wonder - why this should be
>>>>>>>>>> specific to Aspeed I2C and no other I2C controllers implement it?
>>>>>>>>>> IOW, this looks quite generic and every I2C controller should
>>>>>>>>>> have it. Adding it specific to Aspeed suggests that either we
>>>>>>>>>> miss a generic property or this should not be in DT at all
>>>>>>>>>> (because no one else has
>>>>>>>> it...).
>>>>>>>>>>
>>>>>>>>>> Also I wonder, why you wouldn't enable timeout always...
>>>>>>>>>>
>>>>>>>>>> +Cc Wolfram,
>>>>>>>>>> Maybe you know whether bool "timeout" property for one
>>>>>>>>>> controller makes sense? Why we do not have it for all controllers?
>>>>>>>>>>
>>>>>>>>> Because, i2c bus didn’t specific timeout.
>>>>>>>>> But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
>>>>>>>>>
>>>>>>>>> It have definition in SMBus specification.
>>>>>>>>> http://smbus.org/specs/SMBus_3_1_20180319.pdf
>>>>>>>>> You can check Page 18, Note3 that have timeout description.
>>>>>>>>
>>>>>>>> Then you have already property for this - "smbus"?
>>>>>>> To be a property "smbus", that would be a big topic, I saw fsl i2c
>>>>>>> also have this.
>>>>>>> https://github.com/torvalds/linux/blob/master/Documentation/device
>>>>>>> tr
>>>>>>> ee
>>>>>>> /bindings/i2c/i2c-mpc.yaml#L43-L47
>>>>>>> So, I just think the "timeout" property.
>>>>>>
>>>>>> Yeah and this is the only place. It also differs because it allows
>>>>>> actual timeout values.
>>>>> Thanks, So can I still keep the property "aspeed,timeout" here?
>>>>> It is the only place.
>>>>
>>>> No, because none of my concerns above are addressed.
>>>>
>>> Thanks, I realize your concerns.
>>>
>>> So, I modify it like i2c-mpc.yaml
>>> https://github.com/torvalds/linux/blob/master/Documentation/devicetree
>>> /bindings/i2c/i2c-mpc.yaml#L43-L47
>>>
>>>   aspeed,timeout:
>>>     $ref: /schemas/types.yaml#/definitions/uint32
>>>     description: |
>>>       I2C bus timeout in microseconds
>>> Is this way acceptable?
>>
>> So, let's repeat my last questions:
>>
>> 1. Why you wouldn't enable timeout always...
>>
>> You wrote:
>>> http://smbus.org/specs/SMBus_3_1_20180319.pdf
>>> You can check Page 18, Note3 that have timeout description.
>>
>> which indicates you should always use timeout, doesn't it?
> 
> Yes, if board design the bus is connected with SMBUS device, it should enable.
> But in my previous statement, the board design is two multi-master devices connected each other. 

For which you have the property, thus case is solved, isn't it? You want
timeout always except for multi-master?

> And both device is transfer with MCTP protocol. 
> That will not SMBUS protocol. 
> They need have timeout that prevent unexpected un-plug.
> I do the study with smbus in Linux, that will different slave call back. Compare with smbus slave and mctp slave.
> So in this scenario, that is only enable for timeout. 

And the driver knows which protocol it is going to talk and such choice
should not be in DT.

>  
>> 2. Why we do not have it for all controllers with SMBus v3? Why this one is
>> special?
> 
> Not all bus is connected with smbus. Most are i2c device connected in board.
> That will be specific statement for each bus.

That's not the answer to my question. Why other controllers which can be
connected to I2C or SMBus devices do not need this property?

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-07  8:11                           ` Krzysztof Kozlowski
@ 2023-03-07 10:09                             ` Ryan Chen
  2023-03-09  8:51                               ` Krzysztof Kozlowski
  0 siblings, 1 reply; 28+ messages in thread
From: Ryan Chen @ 2023-03-07 10:09 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Wolfram Sang
  Cc: Joel Stanley, Brendan Higgins, Krzysztof Kozlowski,
	Andrew Jeffery, devicetree, Philipp Zabel, Rob Herring,
	Benjamin Herrenschmidt, linux-aspeed, linux-arm-kernel,
	linux-kernel, openbmc, linux-i2c

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Tuesday, March 7, 2023 4:12 PM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
> <wsa@kernel.org>
> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery <andrew@aj.id.au>;
> devicetree@vger.kernel.org; Philipp Zabel <p.zabel@pengutronix.de>; Rob
> Herring <robh+dt@kernel.org>; Benjamin Herrenschmidt
> <benh@kernel.crashing.org>; linux-aspeed@lists.ozlabs.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> openbmc@lists.ozlabs.org; linux-i2c@vger.kernel.org
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> On 06/03/2023 01:48, Ryan Chen wrote:
> > Hello Krzysztof,
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Sent: Sunday, March 5, 2023 5:49 PM
> >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
> >> <wsa@kernel.org>
> >> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
> >> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery
> >> <andrew@aj.id.au>; devicetree@vger.kernel.org; Philipp Zabel
> >> <p.zabel@pengutronix.de>; Rob Herring <robh+dt@kernel.org>; Benjamin
> >> Herrenschmidt <benh@kernel.crashing.org>;
> >> linux-aspeed@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org;
> >> linux-kernel@vger.kernel.org; openbmc@lists.ozlabs.org;
> >> linux-i2c@vger.kernel.org
> >> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 04/03/2023 02:33, Ryan Chen wrote:
> >>> Hello Krzysztof,
> >>>
> >>>> -----Original Message-----
> >>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >>>> Sent: Friday, March 3, 2023 6:41 PM
> >>>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
> >>>> <wsa@kernel.org>
> >>>> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
> >>>> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
> >>>> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery
> >>>> <andrew@aj.id.au>; devicetree@vger.kernel.org; Philipp Zabel
> >>>> <p.zabel@pengutronix.de>; Rob Herring <robh+dt@kernel.org>;
> >>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>;
> >>>> linux-aspeed@lists.ozlabs.org;
> >>>> linux-arm-kernel@lists.infradead.org;
> >>>> linux-kernel@vger.kernel.org; openbmc@lists.ozlabs.org;
> >>>> linux-i2c@vger.kernel.org
> >>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >>>> AST2600-i2cv2
> >>>>
> >>>> On 03/03/2023 11:16, Ryan Chen wrote:
> >>>>>>>>>>> aspeed,timout properites:
> >>>>>>>>>>> For example I2C controller as slave mode, and suddenly
> >>>>>> disconnected.
> >>>>>>>>>>> Slave state machine will keep waiting for master clock in
> >>>>>>>>>>> for rx/tx
> >>>>>>>> transmit.
> >>>>>>>>>>> So it need timeout setting to enable timeout unlock
> >>>>>>>>>>> controller
> >> state.
> >>>>>>>>>>> And in another side. In Master side also need avoid suddenly
> >>>>>>>>>>> slave
> >>>>>>>>>> miss(un-plug), Master will timeout and release the SDA/SCL.
> >>>>>>>>>>>
> >>>>>>>>>>> Do you mean add those description into ore aspeed,timout
> >>>>>>>>>>> properites
> >>>>>>>>>> description?
> >>>>>>>>>>
> >>>>>>>>>> You are describing here one particular feature you want to
> >>>>>>>>>> enable in the driver which looks non-scalable and more
> >>>>>>>>>> difficult to
> >>>>>> configure/use.
> >>>>>>>>>> What I was looking for is to describe the actual
> >>>>>>>>>> configuration you have
> >>>>>> (e.g.
> >>>>>>>>>> multi-master) which leads to enable or disable such feature
> >>>>>>>>>> in your
> >>>>>>>> hardware.
> >>>>>>>>>> Especially that bool value does not scale later to actual
> >>>>>>>>>> timeout values in time (ms)...
> >>>>>>>>>>
> >>>>>>>>>> I don't know I2C that much, but I wonder - why this should be
> >>>>>>>>>> specific to Aspeed I2C and no other I2C controllers implement it?
> >>>>>>>>>> IOW, this looks quite generic and every I2C controller should
> >>>>>>>>>> have it. Adding it specific to Aspeed suggests that either we
> >>>>>>>>>> miss a generic property or this should not be in DT at all
> >>>>>>>>>> (because no one else has
> >>>>>>>> it...).
> >>>>>>>>>>
> >>>>>>>>>> Also I wonder, why you wouldn't enable timeout always...
> >>>>>>>>>>
> >>>>>>>>>> +Cc Wolfram,
> >>>>>>>>>> Maybe you know whether bool "timeout" property for one
> >>>>>>>>>> controller makes sense? Why we do not have it for all controllers?
> >>>>>>>>>>
> >>>>>>>>> Because, i2c bus didn’t specific timeout.
> >>>>>>>>> But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
> >>>>>>>>>
> >>>>>>>>> It have definition in SMBus specification.
> >>>>>>>>> http://smbus.org/specs/SMBus_3_1_20180319.pdf
> >>>>>>>>> You can check Page 18, Note3 that have timeout description.
> >>>>>>>>
> >>>>>>>> Then you have already property for this - "smbus"?
> >>>>>>> To be a property "smbus", that would be a big topic, I saw fsl
> >>>>>>> i2c also have this.
> >>>>>>> https://github.com/torvalds/linux/blob/master/Documentation/devi
> >>>>>>> ce
> >>>>>>> tr
> >>>>>>> ee
> >>>>>>> /bindings/i2c/i2c-mpc.yaml#L43-L47
> >>>>>>> So, I just think the "timeout" property.
> >>>>>>
> >>>>>> Yeah and this is the only place. It also differs because it
> >>>>>> allows actual timeout values.
> >>>>> Thanks, So can I still keep the property "aspeed,timeout" here?
> >>>>> It is the only place.
> >>>>
> >>>> No, because none of my concerns above are addressed.
> >>>>
> >>> Thanks, I realize your concerns.
> >>>
> >>> So, I modify it like i2c-mpc.yaml
> >>> https://github.com/torvalds/linux/blob/master/Documentation/devicetr
> >>> ee
> >>> /bindings/i2c/i2c-mpc.yaml#L43-L47
> >>>
> >>>   aspeed,timeout:
> >>>     $ref: /schemas/types.yaml#/definitions/uint32
> >>>     description: |
> >>>       I2C bus timeout in microseconds Is this way acceptable?
> >>
> >> So, let's repeat my last questions:
> >>
> >> 1. Why you wouldn't enable timeout always...
> >>
> >> You wrote:
> >>> http://smbus.org/specs/SMBus_3_1_20180319.pdf
> >>> You can check Page 18, Note3 that have timeout description.
> >>
> >> which indicates you should always use timeout, doesn't it?
> >
> > Yes, if board design the bus is connected with SMBUS device, it should
> enable.
> > But in my previous statement, the board design is two multi-master devices
> connected each other.
> 
> For which you have the property, thus case is solved, isn't it? You want timeout
> always except for multi-master?

But even if in multi-master board design, no hot-plug requirement.
And it will not need enable the timeout.
That the reason I separate the timeout and multi-master property

> 
> > And both device is transfer with MCTP protocol.
> > That will not SMBUS protocol.
> > They need have timeout that prevent unexpected un-plug.
> > I do the study with smbus in Linux, that will different slave call back.
> Compare with smbus slave and mctp slave.
> > So in this scenario, that is only enable for timeout.
> 
> And the driver knows which protocol it is going to talk and such choice should
> not be in DT.

Sorry, as I understand the i2c driver don't know which protocol is. Due to in i2c driver implement, it just a slave call back function.
i2c_slave_event -> client->slave_cb : up layer to do protocol operate.

> >
> >> 2. Why we do not have it for all controllers with SMBus v3? Why this
> >> one is special?
> >
> > Not all bus is connected with smbus. Most are i2c device connected in board.
> > That will be specific statement for each bus.
> 
> That's not the answer to my question. Why other controllers which can be
> connected to I2C or SMBus devices do not need this property?

For example following is the board specific connection.

Board A                                         Board B
-------------------------                       ------------------------
|i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
|i2c bus#2(master)-> tmp i2c device |          |                       |
|i2c bus#3(master)-> adc i2c device |          |                       |
-------------------------                       ------------------------

Bus#1 is mctp transfer for each BoardA/B. (Not smbus connected)
Bus#2 is i2c device connected.
Bus#3 is i2c device connected.
 

Best regards,
Ryan Chen


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-07 10:09                             ` Ryan Chen
@ 2023-03-09  8:51                               ` Krzysztof Kozlowski
  2023-03-09  9:15                                 ` Ryan Chen
  0 siblings, 1 reply; 28+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-09  8:51 UTC (permalink / raw)
  To: Ryan Chen, Wolfram Sang
  Cc: Joel Stanley, Brendan Higgins, Krzysztof Kozlowski,
	Andrew Jeffery, devicetree, Philipp Zabel, Rob Herring,
	Benjamin Herrenschmidt, linux-aspeed, linux-arm-kernel,
	linux-kernel, openbmc, linux-i2c

On 07/03/2023 11:09, Ryan Chen wrote:
>>>> 2. Why we do not have it for all controllers with SMBus v3? Why this
>>>> one is special?
>>>
>>> Not all bus is connected with smbus. Most are i2c device connected in board.
>>> That will be specific statement for each bus.
>>
>> That's not the answer to my question. Why other controllers which can be
>> connected to I2C or SMBus devices do not need this property?
> 
> For example following is the board specific connection.
> 
> Board A                                         Board B
> -------------------------                       ------------------------
> |i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
> |i2c bus#2(master)-> tmp i2c device |          |                       |
> |i2c bus#3(master)-> adc i2c device |          |                       |
> -------------------------                       ------------------------
> 
> Bus#1 is mctp transfer for each BoardA/B. (Not smbus connected)
> Bus#2 is i2c device connected.
> Bus#3 is i2c device connected.

You are repeating the same stuff for my question. Where do you see on
this diagram here other I2C controller? How does it answer my question?
You keep repeating same and same, so it won't work.



Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-09  8:51                               ` Krzysztof Kozlowski
@ 2023-03-09  9:15                                 ` Ryan Chen
  0 siblings, 0 replies; 28+ messages in thread
From: Ryan Chen @ 2023-03-09  9:15 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Wolfram Sang
  Cc: Joel Stanley, Brendan Higgins, Krzysztof Kozlowski,
	Andrew Jeffery, devicetree, Philipp Zabel, Rob Herring,
	Benjamin Herrenschmidt, linux-aspeed, linux-arm-kernel,
	linux-kernel, openbmc, linux-i2c

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Thursday, March 9, 2023 4:52 PM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Wolfram Sang
> <wsa@kernel.org>
> Cc: Joel Stanley <joel@jms.id.au>; Brendan Higgins
> <brendan.higgins@linux.dev>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Andrew Jeffery <andrew@aj.id.au>;
> devicetree@vger.kernel.org; Philipp Zabel <p.zabel@pengutronix.de>; Rob
> Herring <robh+dt@kernel.org>; Benjamin Herrenschmidt
> <benh@kernel.crashing.org>; linux-aspeed@lists.ozlabs.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> openbmc@lists.ozlabs.org; linux-i2c@vger.kernel.org
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> On 07/03/2023 11:09, Ryan Chen wrote:
> >>>> 2. Why we do not have it for all controllers with SMBus v3? Why
> >>>> this one is special?
> >>>
> >>> Not all bus is connected with smbus. Most are i2c device connected in
> board.
> >>> That will be specific statement for each bus.
> >>
> >> That's not the answer to my question. Why other controllers which can
> >> be connected to I2C or SMBus devices do not need this property?
> >
> > For example following is the board specific connection.
> >
> > Board A                                         Board B
> > -------------------------                       ------------------------
> > |i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
> > |i2c bus#2(master)-> tmp i2c device |          |
> |
> > |i2c bus#3(master)-> adc i2c device |          |
> |
> > -------------------------                       ------------------------
> >
> > Bus#1 is mctp transfer for each BoardA/B. (Not smbus connected)
> > Bus#2 is i2c device connected.
> > Bus#3 is i2c device connected.
> 
> You are repeating the same stuff for my question. Where do you see on this
> diagram here other I2C controller? How does it answer my question?
> You keep repeating same and same, so it won't work.
> 
Sorry, my mis-understand.  
I don't see many controllers support for timeout feature.
I do study with other controllers that is implement it by sw impelement.
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-mlxbf.c#L302-L307
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-img-scb.c#L52-L55
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-omap.c#L588-L596

So I asking for aspeed,timeout property, If it not acceptable, I will default enable timeout in driver.
Thanks your guidance.



Best regards,
Ryan Chen

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-03  8:20       ` Krzysztof Kozlowski
  2023-03-03  8:28         ` Ryan Chen
@ 2023-03-12 12:33         ` Andi Shyti
  1 sibling, 0 replies; 28+ messages in thread
From: Andi Shyti @ 2023-03-12 12:33 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Ryan Chen, Wolfram Sang, Joel Stanley, Brendan Higgins,
	Krzysztof Kozlowski, Andrew Jeffery, devicetree, Philipp Zabel,
	Rob Herring, Benjamin Herrenschmidt, linux-aspeed,
	linux-arm-kernel, linux-kernel, openbmc, linux-i2c

Hi Krzysztof and Ryan,

> >>> +  aspeed,timeout:
> >>> +    type: boolean
> >>> +    description: I2C bus timeout enable for master/slave mode
> >>
> >> Nothing improved here in regards to my last comment.
> > 
> > Yes, as I know your require is about " DT binding to represent hardware setup"
> > So I add more description about aspeed,timeout as blow.
> > 
> > ASPEED SOC chip is server product, i2c bus may have fingerprint connect to another board. And also support hotplug.
> > The following is board-specific design example.
> > Board A                                         Board B
> > -------------------------                       ------------------------
> > |i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
> > |i2c bus#2(master)-> tmp i2c device |          |                       |
> > |i2c bus#3(master)-> adc i2c device |          |                       |
> > -------------------------                       ------------------------
> > 
> > aspeed,timout properites:
> > For example I2C controller as slave mode, and suddenly disconnected.
> > Slave state machine will keep waiting for master clock in for rx/tx transmit.
> > So it need timeout setting to enable timeout unlock controller state.
> > And in another side. In Master side also need avoid suddenly slave miss(un-plug), Master will timeout and release the SDA/SCL.
> > 
> > Do you mean add those description into ore aspeed,timout properites description?
> 
> You are describing here one particular feature you want to enable in the
> driver which looks non-scalable and more difficult to configure/use.
> What I was looking for is to describe the actual configuration you have
> (e.g. multi-master) which leads to enable or disable such feature in
> your hardware. Especially that bool value does not scale later to actual
> timeout values in time (ms)...
> 
> I don't know I2C that much, but I wonder - why this should be specific
> to Aspeed I2C and no other I2C controllers implement it? IOW, this looks
> quite generic and every I2C controller should have it. Adding it
> specific to Aspeed suggests that either we miss a generic property or
> this should not be in DT at all (because no one else has it...).

this property is missing in the i2c devicetree property and
because this is the second driver needing it, I think it should
be added.

To be clear, this timeout means that the SCL is kept low for some
number of milliseconds in order to force the slave to enter a
wait state. This is done when the master has some particular
needs as Ryan is describing.

It's defined in the i2c specification, while smbus defines it in
a range from 25 to 35 ms.

In any case it's not a boolean value unless the controller has it
defined internally by the firmware.

So... nack! Please, hold a bit, I'm sending a patch. 

Andi

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-02-26  3:13 ` [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2 Ryan Chen
  2023-02-26  7:04   ` Jeremy Kerr
  2023-02-27  8:25   ` Krzysztof Kozlowski
@ 2023-03-18  9:09   ` Andi Shyti
  2023-03-19  2:05     ` Ryan Chen
  2 siblings, 1 reply; 28+ messages in thread
From: Andi Shyti @ 2023-03-18  9:09 UTC (permalink / raw)
  To: Ryan Chen
  Cc: Andrew Jeffery, Brendan Higgins, Benjamin Herrenschmidt,
	Joel Stanley, Rob Herring, Krzysztof Kozlowski, Philipp Zabel,
	linux-i2c, openbmc, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel

Hi Ryan,

On Sun, Feb 26, 2023 at 11:13:20AM +0800, Ryan Chen wrote:
> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout
> aspeed,xfer-mode description for ast2600-i2cv2.
> 
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> index f597f73ccd87..75de3ce41cf5 100644
> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> @@ -49,6 +49,25 @@ properties:
>      description:
>        states that there is another master active on this bus
>  
> +  aspeed,timeout:
> +    type: boolean
> +    description: I2C bus timeout enable for master/slave mode

Finally you can proceed with this. Please remove "aspeed,timeout"
and use "i2c-scl-has-clk-low-timeout" instead.

Andi

^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  2023-03-18  9:09   ` Andi Shyti
@ 2023-03-19  2:05     ` Ryan Chen
  0 siblings, 0 replies; 28+ messages in thread
From: Ryan Chen @ 2023-03-19  2:05 UTC (permalink / raw)
  To: Andi Shyti
  Cc: Andrew Jeffery, Brendan Higgins, Benjamin Herrenschmidt,
	Joel Stanley, Rob Herring, Krzysztof Kozlowski, Philipp Zabel,
	linux-i2c, openbmc, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel

Hello,

> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: Saturday, March 18, 2023 5:10 PM
> To: Ryan Chen <ryan_chen@aspeedtech.com>
> Cc: Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins
> <brendan.higgins@linux.dev>; Benjamin Herrenschmidt
> <benh@kernel.crashing.org>; Joel Stanley <joel@jms.id.au>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Philipp Zabel
> <p.zabel@pengutronix.de>; linux-i2c@vger.kernel.org;
> openbmc@lists.ozlabs.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-aspeed@lists.ozlabs.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> Hi Ryan,
> 
> On Sun, Feb 26, 2023 at 11:13:20AM +0800, Ryan Chen wrote:
> > Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout
> > aspeed,xfer-mode description for ast2600-i2cv2.
> >
> > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> > ---
> >  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
> +++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > index f597f73ccd87..75de3ce41cf5 100644
> > --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > @@ -49,6 +49,25 @@ properties:
> >      description:
> >        states that there is another master active on this bus
> >
> > +  aspeed,timeout:
> > +    type: boolean
> > +    description: I2C bus timeout enable for master/slave mode
> 
> Finally you can proceed with this. Please remove "aspeed,timeout"
> and use "i2c-scl-has-clk-low-timeout" instead.

Thanks a lot, I will start progress this. 

Best Regards
Ryan

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2023-03-19  2:05 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-26  3:13 [PATCH v6 0/2] Add ASPEED AST2600 I2Cv2 controller driver Ryan Chen
2023-02-26  3:13 ` [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2 Ryan Chen
2023-02-26  7:04   ` Jeremy Kerr
2023-02-27  4:12     ` Ryan Chen
2023-02-27  5:40       ` Jeremy Kerr
2023-03-01  3:02         ` Ryan Chen
2023-03-01  3:23           ` Jeremy Kerr
2023-03-01  3:40             ` Ryan Chen
2023-02-27  8:25   ` Krzysztof Kozlowski
2023-03-01  5:57     ` Ryan Chen
2023-03-03  8:20       ` Krzysztof Kozlowski
2023-03-03  8:28         ` Ryan Chen
2023-03-03  8:50           ` Krzysztof Kozlowski
2023-03-03  8:55             ` Ryan Chen
2023-03-03  9:26               ` Krzysztof Kozlowski
2023-03-03 10:16                 ` Ryan Chen
2023-03-03 10:41                   ` Krzysztof Kozlowski
2023-03-04  1:33                     ` Ryan Chen
2023-03-05  9:49                       ` Krzysztof Kozlowski
2023-03-06  0:48                         ` Ryan Chen
2023-03-07  8:11                           ` Krzysztof Kozlowski
2023-03-07 10:09                             ` Ryan Chen
2023-03-09  8:51                               ` Krzysztof Kozlowski
2023-03-09  9:15                                 ` Ryan Chen
2023-03-12 12:33         ` Andi Shyti
2023-03-18  9:09   ` Andi Shyti
2023-03-19  2:05     ` Ryan Chen
2023-02-26  3:13 ` [PATCH v6 2/2] i2c: aspeed: support ast2600 i2cv new register mode driver Ryan Chen

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).