[PATCH v16 0/3] Add ASPEED AST2600 I2Cv2 controller driver

[PATCH v16 0/3] Add ASPEED AST2600 I2Cv2 controller driver

[Ryan Chen]

This series add AST2600 i2cv2 new register set driver. The i2cv2 driver
is new register set that have new clock divider option for more flexiable
generation. And also have separate i2c controller and target register
set for control, patch #2 is i2c controller driver only, patch #3 is add
i2c target mode driver.

The legacy register layout is mix controller/target 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 controller and target mode control.
-Support 4 individual DMA buffers for controller Tx and Rx,
target 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 controller DMA command mode.
-Bus auto timeout for new controller/target 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(controller/target)<=fingerprint=>i2c bus#x (controller/target)|
|i2c bus#2(controller)-> tmp i2c device |     |                          |
|i2c bus#3(controller)-> adc i2c device |     |                          |
-------------------------                     ------------------------

i2c-scl-clk-low-timeout-us:
For example I2C controller as slave mode, and suddenly disconnected.
Slave state machine will keep waiting for controller 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,enable-dma:
For example The bus#1 have trunk data needed for transfer,
it can enable bus dma mode transfer, it can reduce cpu utilized.
Others bus bus#2/3 use defautl buffer mode.

aspeed,enable-byte:
Force i2c controller use byte mode transfer. the byte mode transfer
will send i2c data each byte by byte, inlcude address read/write.

v16:
-aspeed,i2c.yaml: add aspeed,enable-byte properties for force byte mode.
-i2c-ast2600.c
 -change include asm/unaligned.h to linux/unaligned.h.
 -add reset timeout councter when slave active timeout.
 -modify issue i2c_recovery_bus before slave re-enable.
 -add aspeed,enable-byte properties.

v15:
-i2c-ast2600.c
 -add include unaligned.h
 -rename all master -> controller, slave -> target.
 -keep multi-master to align property.
 -remove no used element in ast2600_i2c_bus.
v14:
-aspeed,i2c.yaml
 -v13 change people reviewed-by tag, v14 fixed to original people tag,
modify to Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
 -struct ast2600_i2c_bus layout optimal.
 -ast2600_select_i2c_clock refine.
 -ast2600_i2c_recover_bus overridden fix.
 -dma_mapping_error() returned error code shadowed modify.
 -buffer register in a 4-byte aligned simplified
 -remove smbus alert

v13:
 -separate i2c master and slave driver to be two patchs.
 -modify include header list, add bits.h include. remove of*.h
 -modify (((x) >> 24) & GENMASK(5, 0)) to (((x) & GENMASK(29, 24)) >> 24)
 -modify ast2600_select_i2c_clock function implement.
 -modify ast2600_i2c_recover_bus function u32 claim to
u32 state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);

v12:
-aspeed,i2c.yaml
 -add Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
-i2c-ast2600.c
 -update include by alphabetical order
 -make just a one TAB and put the last two lines on the single one
 -remove no used timing_table structre
 -remove enum explicit assinment
 -rewritten to avoid this and using loop in ast2600_select_i2c_clock
 -use GENMASK for most 0xffff
 -remove too many parentheses
 -use str_read_write replace read write string
 -remove redundant blank line after ast2600_i2c_bus_of_table
 -fix wrong multi-line style of the comment
 -use macro for i2c standard speeds
 -remove useless noise dev_info

v11:
-aspeed,i2c.yaml
 -no change, the same with v10.
-i2c-ast2600.c
 -modify alert_enable from int -> boolean.
 -modify dbg string recovery -> recover.
 -remove no need to init 0.
 -remove new line after break.
 -remove unneeded empty line.
 -modify dma_alloc_coherent to dmam_alloc_coherent
 -modify probe nomem return dev_err_probe
 -modify i2c_add_adapter to devm_i2c_adapter
 -modify checkpatch: Alignment should match open parenthesis
 -modify checkpatch: braces {} should be used on all arms of this statement
 -modify checkpatch: Unbalanced braces around else statement

v10:
-aspeed,i2c.yaml
 -move unevaluatedProperties after allOf.
 -remove extra one blank line.
-i2c-ast2600.c
 -no change, the same with v8.

v9:
-aspeed,i2c.yaml
 -backoff to v7.
  -no fix typo in maintainer's name and email. this would be another patch.
  -no remove address-cells, size-cells, this would be another patch.
 -use aspeed,enable-dma property instead of aspeed,xfer-mode selection.
 -fix allOf and else false properties for aspeed,ast2600-i2cv2.
-i2c-ast2600.c
 -no change, the same with v8

v8:
-aspeed,i2c.yaml
 -modify commit message.
  -Fix typo in maintainer's name and email.
 -remove address-cells, size-cells.
-i2c-ast2600.c
 -move "i2c timeout counter" comment description before property_read.
 -remove redundant code "return ret" in probe end.

v7:
-aspeed,i2c.yaml
 -Update ASPEED I2C maintainers email.
 -use aspeed,enable-dma property instead of aspeed,xfer-mode selection.
 -fix allOf and else false properties for aspeed,ast2600-i2cv2.
-i2c-ast2600.c
 -remove aspeed,xfer-mode instead of aspeed,enable-dma mode. buffer mode
is default.
 -remove aspeed,timeout instead of i2c-scl-clk-low-timeout-us for
timeout setting.

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 (3):
  dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  i2c: aspeed: support AST2600 i2c new register mode driver
  i2c: aspeed: support AST2600 i2c new register target mode driver

 .../devicetree/bindings/i2c/aspeed,i2c.yaml   |   58 +
 drivers/i2c/busses/Kconfig                    |   11 +
 drivers/i2c/busses/Makefile                   |    1 +
 drivers/i2c/busses/i2c-ast2600.c              | 1565 +++++++++++++++++
 4 files changed, 1635 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ast2600.c

-- 
2.34.1

[PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma
and description for ast2600-i2cv2.

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

diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
index 5b9bd2feda3b..356033d18f90 100644
--- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
@@ -44,12 +44,60 @@ properties:
     description: frequency of the bus clock in Hz defaults to 100 kHz when not
       specified
 
+  multi-master:
+    type: boolean
+    description:
+      states that there is another master active on this bus
+
+  aspeed,enable-dma:
+    type: boolean
+    description: |
+      I2C bus enable dma mode transfer.
+
+      ASPEED ast2600 platform equipped with 16 I2C controllers that share a
+      single DMA engine. DTS files can specify the data transfer mode to/from
+      the device, either DMA or programmed I/O. However, hardware limitations
+      may require a DTS to manually allocate which controller can use DMA mode.
+      The "aspeed,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
+      enable DMA mode for that one controller.
+
+  aspeed,enable-byte:
+    type: boolean
+    description: |
+      I2C bus enable byte mode transfer.
+
+  aspeed,global-regs:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: The phandle of i2c global register node.
+
 required:
   - reg
   - compatible
   - clocks
   - resets
 
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: aspeed,ast2600-i2cv2
+
+    then:
+      properties:
+        reg:
+          minItems: 2
+      required:
+        - aspeed,global-regs
+    else:
+      properties:
+        aspeed,global-regs: false
+        aspeed,enable-dma: false
+
 unevaluatedProperties: false
 
 examples:
@@ -66,3 +114,13 @@ examples:
       interrupts = <0>;
       interrupt-parent = <&i2c_ic>;
     };
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    i2c1: i2c@80 {
+      compatible = "aspeed,ast2600-i2cv2";
+      reg = <0x80 0x80>, <0xc00 0x20>;
+      aspeed,global-regs = <&i2c_global>;
+      clocks = <&syscon ASPEED_CLK_APB>;
+      resets = <&syscon ASPEED_RESET_I2C>;
+      interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
+    };
-- 
2.34.1

[PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new register mode driver

[Ryan Chen]

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 new register mode have separate register
set to control i2c controller and target. This patch is for i2c
controller mode driver.

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

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index fc438f445771..32d96d5c8f5d 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -402,6 +402,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 1c2a4510abe4..ebfc377687b1 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -38,6 +38,7 @@ 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_AMD_ASF)	+= i2c-amd-asf-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-y			:= i2c-at91-core.o i2c-at91-master.o
 i2c-at91-$(CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL)	+= i2c-at91-slave.o
diff --git a/drivers/i2c/busses/i2c-ast2600.c b/drivers/i2c/busses/i2c-ast2600.c
new file mode 100644
index 000000000000..bfac507693dd
--- /dev/null
+++ b/drivers/i2c/busses/i2c-ast2600.c
@@ -0,0 +1,1036 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ASPEED AST2600 new register set I2C controller driver
+ *
+ * Copyright (C) ASPEED Technology Inc.
+ */
+#include <linux/array_size.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/i2c-smbus.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/minmax.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+#include <linux/string_helpers.h>
+#include <linux/unaligned.h>
+
+#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)
+/*
+ * 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 I2CCG_DIV_CTRL 0xC6411208
+
+/* 0x00 : I2CC Controller/Target 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 Controller/Target 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 Controller/Target 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 Controller/Target Pool Buffer Control Register  */
+#define AST2600_I2CC_BUFF_CTRL		0x0C
+#define AST2600_I2CC_GET_RX_BUF_LEN(x)      (((x) & GENMASK(29, 24)) >> 24)
+#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) & GENMASK(12, 8)) >> 8) + 1)
+
+/* 0x10 : I2CM Controller Interrupt Control Register */
+#define AST2600_I2CM_IER			0x10
+/* 0x14 : I2CM Controller 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 Controller 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 Controller 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 Target Interrupt Control Register   */
+#define AST2600_I2CS_IER			0x20
+/* 0x24 : I2CS Target 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 Target 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_SET_TX_DMA_LEN(x)	((((x) - 1) & GENMASK(11, 0)) | BIT(15))
+
+/* I2CM Controller DMA Tx Buffer Register   */
+#define AST2600_I2CM_TX_DMA			0x30
+/* I2CM Controller DMA Rx Buffer Register	*/
+#define AST2600_I2CM_RX_DMA			0x34
+/* I2CS Target DMA Tx Buffer Register   */
+#define AST2600_I2CS_TX_DMA			0x38
+/* I2CS Target 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) & GENMASK(28, 16)) >> 16)
+
+/* 0x40 : Target 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_TARGET_MSG_BUF_SIZE		256
+
+#define AST2600_I2C_DMA_SIZE		4096
+
+#define CONTROLLER_TRIGGER_LAST_STOP	(AST2600_I2CM_RX_CMD_LAST | AST2600_I2CM_STOP_CMD)
+#define TARGET_TRIGGER_CMD	(AST2600_I2CS_ACTIVE_ALL | AST2600_I2CS_PKT_MODE_EN)
+
+#define AST_I2C_TIMEOUT_CLK		0x1
+
+enum xfer_mode {
+	BYTE_MODE,
+	BUFF_MODE,
+	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;
+	struct clk		*clk;
+	struct i2c_timings	timing_info;
+	struct completion	cmd_complete;
+	struct i2c_msg		*msgs;
+	u8			*controller_dma_safe_buf;
+	dma_addr_t		controller_dma_addr;
+	u32			apb_clk;
+	u32			timeout;
+	int			irq;
+	int			cmd_err;
+	int			msgs_index;
+	int			msgs_count;
+	int			controller_xfer_cnt;
+	size_t			buf_index;
+	size_t			buf_size;
+	enum xfer_mode		mode;
+	bool			multi_master;
+	/* Buffer mode */
+	void __iomem		*buf_base;
+	struct i2c_smbus_alert_setup	alert_data;
+};
+
+static u32 ast2600_select_i2c_clock(struct ast2600_i2c_bus *i2c_bus)
+{
+	unsigned long base_clk[16];
+	int baseclk_idx = 0;
+	int divisor = 0;
+	u32 clk_div_reg;
+	u32 scl_low;
+	u32 scl_high;
+	u32 data;
+
+	regmap_read(i2c_bus->global_regs, AST2600_I2CG_CLK_DIV_CTRL, &clk_div_reg);
+
+	for (int i = 0; i < ARRAY_SIZE(base_clk); i++) {
+		if (i == 0)
+			base_clk[i] = i2c_bus->apb_clk;
+		else if (i < 5)
+			base_clk[i] = (i2c_bus->apb_clk * 2) /
+			   (((clk_div_reg >> ((i - 1) * 8)) & GENMASK(7, 0)) + 2);
+		else
+			base_clk[i] = base_clk[4] >> (i - 5);
+
+		if ((base_clk[i] / i2c_bus->timing_info.bus_freq_hz) <= 32) {
+			baseclk_idx = i;
+			divisor = DIV_ROUND_UP(base_clk[i], i2c_bus->timing_info.bus_freq_hz);
+			break;
+		}
+	}
+	baseclk_idx = min(baseclk_idx, 15);
+	divisor = min(divisor, 32);
+	scl_low = min(divisor * 9 / 16 - 1, 15);
+	scl_high = (divisor - scl_low - 2) & GENMASK(3, 0);
+	data = (scl_high - 1) << 20 | scl_high << 16 | scl_low << 12 | baseclk_idx;
+	if (i2c_bus->timeout) {
+		data |= AST2600_I2CC_TOUTBASECLK(AST_I2C_TIMEOUT_CLK);
+		data |= AST2600_I2CC_TTIMEOUT(i2c_bus->timeout);
+	}
+
+	return data;
+}
+
+static u8 ast2600_i2c_recover_bus(struct ast2600_i2c_bus *i2c_bus)
+{
+	u32 state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
+	int ret = 0;
+	u32 ctrl;
+	int r;
+
+	dev_dbg(i2c_bus->dev, "%d-bus recovery bus [%x]\n", i2c_bus->adap.nr, state);
+
+	ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+	/* Disable controller */
+	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");
+			writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+			return -ETIMEDOUT;
+		} else if (i2c_bus->cmd_err) {
+			dev_dbg(i2c_bus->dev, "recovery error\n");
+			ret = -EPROTO;
+		}
+	}
+
+	/* Recovery done */
+	state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
+	if (state & AST2600_I2CC_BUS_BUSY_STS) {
+		dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n", state);
+		ret = -EPROTO;
+	}
+
+	/* restore original controller setting */
+	writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+	return ret;
+}
+
+static int ast2600_i2c_setup_dma_tx(u32 cmd, struct ast2600_i2c_bus *i2c_bus)
+{
+	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
+	int xfer_len;
+	int ret;
+
+	cmd |= AST2600_I2CM_PKT_EN;
+	xfer_len = msg->len - i2c_bus->controller_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;
+
+	if (cmd & AST2600_I2CM_START_CMD) {
+		cmd |= AST2600_I2CM_PKT_ADDR(msg->addr);
+		i2c_bus->controller_dma_safe_buf = i2c_get_dma_safe_msg_buf(msg, 1);
+		if (!i2c_bus->controller_dma_safe_buf)
+			return -ENOMEM;
+		i2c_bus->controller_dma_addr =
+			dma_map_single(i2c_bus->dev, i2c_bus->controller_dma_safe_buf,
+				       msg->len, DMA_TO_DEVICE);
+		ret = dma_mapping_error(i2c_bus->dev, i2c_bus->controller_dma_addr);
+		if (ret) {
+			i2c_put_dma_safe_msg_buf(i2c_bus->controller_dma_safe_buf, msg, false);
+			i2c_bus->controller_dma_safe_buf = NULL;
+			return ret;
+		}
+	}
+
+	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);
+		writel(i2c_bus->controller_dma_addr + i2c_bus->controller_xfer_cnt,
+		       i2c_bus->reg_base + AST2600_I2CM_TX_DMA);
+	}
+
+	writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
+
+	return 0;
+}
+
+static int ast2600_i2c_setup_buff_tx(u32 cmd, struct ast2600_i2c_bus *i2c_bus)
+{
+	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
+	int xfer_len = msg->len - i2c_bus->controller_xfer_cnt;
+	u32 wbuf_dword;
+	int i;
+
+	cmd |= AST2600_I2CM_PKT_EN;
+
+	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;
+
+	if (cmd & AST2600_I2CM_START_CMD)
+		cmd |= AST2600_I2CM_PKT_ADDR(msg->addr);
+
+	if (xfer_len) {
+		cmd |= AST2600_I2CM_TX_BUFF_EN | AST2600_I2CM_TX_CMD;
+		/*
+		 * The controller's buffer register supports dword writes only.
+		 * Therefore, write dwords to the buffer register in a 4-byte aligned,
+		 * and write the remaining unaligned data at the end.
+		 */
+		for (i = 0; i < xfer_len; i += 4) {
+			int xfer_cnt = i2c_bus->controller_xfer_cnt + i;
+
+			switch (min(xfer_len - i, 4) % 4) {
+			case 1:
+				wbuf_dword = msg->buf[xfer_cnt];
+				break;
+			case 2:
+				wbuf_dword = get_unaligned_le16(&msg->buf[xfer_cnt]);
+				break;
+			case 3:
+				wbuf_dword = get_unaligned_le24(&msg->buf[xfer_cnt]);
+				break;
+			default:
+				wbuf_dword = get_unaligned_le32(&msg->buf[xfer_cnt]);
+				break;
+			}
+			writel(wbuf_dword, i2c_bus->buf_base + i);
+		}
+		writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len),
+		       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+	}
+
+	writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
+
+	return 0;
+}
+
+static int ast2600_i2c_setup_byte_tx(u32 cmd, struct ast2600_i2c_bus *i2c_bus)
+{
+	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
+	int xfer_len;
+
+	xfer_len = msg->len - i2c_bus->controller_xfer_cnt;
+
+	cmd |= AST2600_I2CM_PKT_EN;
+
+	if (cmd & AST2600_I2CM_START_CMD)
+		cmd |= AST2600_I2CM_PKT_ADDR(msg->addr);
+
+	if ((i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) &&
+	    ((i2c_bus->controller_xfer_cnt + 1) == msg->len))
+		cmd |= AST2600_I2CM_STOP_CMD;
+
+	if (xfer_len) {
+		cmd |= AST2600_I2CM_TX_CMD;
+		writel(msg->buf[i2c_bus->controller_xfer_cnt],
+		       i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
+	}
+
+	writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
+
+	return 0;
+}
+
+static int ast2600_i2c_setup_dma_rx(struct ast2600_i2c_bus *i2c_bus)
+{
+	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
+	int xfer_len;
+	u32 cmd;
+	int ret;
+
+	cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(msg->addr) |
+	      AST2600_I2CM_START_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 |= CONTROLLER_TRIGGER_LAST_STOP;
+	}
+	writel(AST2600_I2CM_SET_RX_DMA_LEN(xfer_len - 1), i2c_bus->reg_base + AST2600_I2CM_DMA_LEN);
+	i2c_bus->controller_dma_safe_buf = i2c_get_dma_safe_msg_buf(msg, 1);
+	if (!i2c_bus->controller_dma_safe_buf)
+		return -ENOMEM;
+	i2c_bus->controller_dma_addr =
+		dma_map_single(i2c_bus->dev, i2c_bus->controller_dma_safe_buf,
+			       msg->len, DMA_FROM_DEVICE);
+	ret = dma_mapping_error(i2c_bus->dev, i2c_bus->controller_dma_addr);
+	if (ret) {
+		i2c_put_dma_safe_msg_buf(i2c_bus->controller_dma_safe_buf, msg, false);
+		i2c_bus->controller_dma_safe_buf = NULL;
+		return ret;
+	}
+	writel(i2c_bus->controller_dma_addr, i2c_bus->reg_base + AST2600_I2CM_RX_DMA);
+
+	writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
+
+	return 0;
+}
+
+static int ast2600_i2c_setup_buff_rx(struct ast2600_i2c_bus *i2c_bus)
+{
+	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
+	int xfer_len;
+	u32 cmd;
+
+	cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(msg->addr) |
+	      AST2600_I2CM_START_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 |= CONTROLLER_TRIGGER_LAST_STOP;
+	}
+	writel(AST2600_I2CC_SET_RX_BUF_LEN(xfer_len), i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+
+	writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
+
+	return 0;
+}
+
+static int ast2600_i2c_setup_byte_rx(struct ast2600_i2c_bus *i2c_bus)
+{
+	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
+	u32 cmd;
+
+	cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(msg->addr) |
+	      AST2600_I2CM_START_CMD | AST2600_I2CM_RX_CMD;
+
+	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 |= CONTROLLER_TRIGGER_LAST_STOP;
+	}
+
+	writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
+
+	return 0;
+}
+
+static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus)
+{
+	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
+
+	/* send start */
+	dev_dbg(i2c_bus->dev, "[%d] %s %d byte%s %s 0x%02x\n",
+		i2c_bus->msgs_index, str_read_write(msg->flags & I2C_M_RD),
+		msg->len, str_plural(msg->len),
+		msg->flags & I2C_M_RD ? "from" : "to", msg->addr);
+
+	i2c_bus->controller_xfer_cnt = 0;
+	i2c_bus->buf_index = 0;
+
+	if (msg->flags & I2C_M_RD) {
+		if (i2c_bus->mode == DMA_MODE)
+			return ast2600_i2c_setup_dma_rx(i2c_bus);
+		else if (i2c_bus->mode == BUFF_MODE)
+			return ast2600_i2c_setup_buff_rx(i2c_bus);
+		else
+			return ast2600_i2c_setup_byte_rx(i2c_bus);
+	} else {
+		if (i2c_bus->mode == DMA_MODE)
+			return ast2600_i2c_setup_dma_tx(AST2600_I2CM_START_CMD, i2c_bus);
+		else if (i2c_bus->mode == BUFF_MODE)
+			return ast2600_i2c_setup_buff_tx(AST2600_I2CM_START_CMD, i2c_bus);
+		else
+			return ast2600_i2c_setup_byte_tx(AST2600_I2CM_START_CMD, i2c_bus);
+	}
+}
+
+static int ast2600_i2c_irq_err_to_errno(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_controller_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->controller_xfer_cnt += xfer_len;
+
+		if (i2c_bus->controller_xfer_cnt == msg->len) {
+			if (i2c_bus->mode == DMA_MODE) {
+				dma_unmap_single(i2c_bus->dev, i2c_bus->controller_dma_addr,
+						 msg->len, DMA_TO_DEVICE);
+				i2c_put_dma_safe_msg_buf(i2c_bus->controller_dma_safe_buf,
+							 msg, true);
+				i2c_bus->controller_dma_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 {
+			if (i2c_bus->mode == DMA_MODE)
+				ast2600_i2c_setup_dma_tx(0, i2c_bus);
+			else if (i2c_bus->mode == BUFF_MODE)
+				ast2600_i2c_setup_buff_tx(0, i2c_bus);
+			else
+				ast2600_i2c_setup_byte_tx(0, i2c_bus);
+		}
+		break;
+	case AST2600_I2CM_RX_DONE:
+	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->controller_xfer_cnt + i] =
+					readb(i2c_bus->buf_base + 0x10 + i);
+		} else {
+			xfer_len = 1;
+			msg->buf[i2c_bus->controller_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 = min_t(unsigned int, msg->buf[0], I2C_SMBUS_BLOCK_MAX);
+			msg->len += ((msg->flags & I2C_CLIENT_PEC) ? 2 : 1);
+			msg->flags &= ~I2C_M_RECV_LEN;
+		}
+		i2c_bus->controller_xfer_cnt += xfer_len;
+
+		if (i2c_bus->controller_xfer_cnt == msg->len) {
+			if (i2c_bus->mode == DMA_MODE) {
+				dma_unmap_single(i2c_bus->dev, i2c_bus->controller_dma_addr,
+						 msg->len, DMA_FROM_DEVICE);
+				i2c_put_dma_safe_msg_buf(i2c_bus->controller_dma_safe_buf,
+							 msg, true);
+				i2c_bus->controller_dma_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->controller_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 |= CONTROLLER_TRIGGER_LAST_STOP;
+				}
+				writel(AST2600_I2CM_SET_RX_DMA_LEN(xfer_len - 1),
+				       i2c_bus->reg_base + AST2600_I2CM_DMA_LEN);
+				writel(i2c_bus->controller_dma_addr + i2c_bus->controller_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->controller_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 |= CONTROLLER_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->controller_xfer_cnt + 1) == msg->len)) {
+					cmd |= CONTROLLER_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_controller_irq(struct ast2600_i2c_bus *i2c_bus)
+{
+	u32 sts = readl(i2c_bus->reg_base + AST2600_I2CM_ISR);
+	u32 ctrl;
+
+	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;
+	}
+
+	i2c_bus->cmd_err = ast2600_i2c_irq_err_to_errno(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_controller_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;
+
+	return IRQ_RETVAL(ast2600_i2c_controller_irq(i2c_bus));
+}
+
+static int ast2600_i2c_controller_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;
+
+	if (!i2c_bus->multi_master &&
+	    (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) & AST2600_I2CC_BUS_BUSY_STS)) {
+		ret = ast2600_i2c_recover_bus(i2c_bus);
+		if (ret)
+			return ret;
+	}
+
+	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);
+	if (ret)
+		goto controller_out;
+	timeout = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout);
+	if (timeout == 0) {
+		u32 ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+		dev_dbg(i2c_bus->dev, "timeout isr[%x], sts[%x]\n",
+			readl(i2c_bus->reg_base + AST2600_I2CM_ISR),
+			readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));
+		writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+		writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+		if (i2c_bus->multi_master &&
+		    (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) &
+		    AST2600_I2CC_BUS_BUSY_STS))
+			ast2600_i2c_recover_bus(i2c_bus);
+
+		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);
+
+controller_out:
+	if (i2c_bus->mode == DMA_MODE) {
+		kfree(i2c_bus->controller_dma_safe_buf);
+		i2c_bus->controller_dma_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);
+
+	i2c_bus->multi_master = device_property_read_bool(&pdev->dev, "multi-master");
+	if (!i2c_bus->multi_master)
+		fun_ctrl |= AST2600_I2CC_MULTI_MASTER_DIS;
+
+	/* Enable Controller Mode */
+	writel(fun_ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+	/* disable target 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(GENMASK(27, 0), i2c_bus->reg_base + AST2600_I2CM_ISR);
+}
+
+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 = {
+	.xfer = ast2600_i2c_controller_xfer,
+	.functionality = ast2600_i2c_functionality,
+};
+
+static int ast2600_i2c_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ast2600_i2c_bus *i2c_bus;
+	struct resource *res;
+	u32 global_ctrl;
+	int ret;
+
+	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(dev_of_node(dev), "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->dev = dev;
+	i2c_bus->mode = BUFF_MODE;
+
+	if (device_property_read_bool(dev, "aspeed,enable-dma"))
+		i2c_bus->mode = DMA_MODE;
+
+	if (device_property_read_bool(dev, "aspeed,enable-byte"))
+		i2c_bus->mode = BYTE_MODE;
+
+	if (i2c_bus->mode == BUFF_MODE) {
+		i2c_bus->buf_base = devm_platform_get_and_ioremap_resource(pdev, 1, &res);
+		if (IS_ERR(i2c_bus->buf_base))
+			i2c_bus->mode = BYTE_MODE;
+		else
+			i2c_bus->buf_size = resource_size(res) / 2;
+	}
+
+	/*
+	 * i2c timeout counter: use base clk4 1Mhz,
+	 * per unit: 1/(1000/1024) = 1024us
+	 */
+	ret = device_property_read_u32(dev, "i2c-scl-clk-low-timeout-us", &i2c_bus->timeout);
+	if (!ret)
+		i2c_bus->timeout /= 1024;
+
+	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);
+
+	i2c_parse_fw_timings(i2c_bus->dev, &i2c_bus->timing_info, true);
+
+	/* 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;
+	device_set_node(&i2c_bus->adap.dev, dev_fwnode(dev));
+	i2c_bus->adap.algo_data = i2c_bus;
+	strscpy(i2c_bus->adap.name, pdev->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);
+
+	writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
+	       i2c_bus->reg_base + AST2600_I2CM_IER);
+
+	ret = devm_i2c_add_adapter(dev, &i2c_bus->adap);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void 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);
+	reset_control_assert(i2c_bus->rst);
+}
+
+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 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

[PATCH v16 3/3] i2c: aspeed: support AST2600 i2c new register target mode driver

[Ryan Chen]

This patch is for i2c new register target mode driver.

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 drivers/i2c/busses/i2c-ast2600.c | 569 +++++++++++++++++++++++++++++--
 1 file changed, 549 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ast2600.c b/drivers/i2c/busses/i2c-ast2600.c
index bfac507693dd..528575b25f1e 100644
--- a/drivers/i2c/busses/i2c-ast2600.c
+++ b/drivers/i2c/busses/i2c-ast2600.c
@@ -81,6 +81,7 @@
 #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))
+#define AST2600_I2CC_AC_TIMING_MASK		GENMASK(23, 0)
 
 /* 0x08 : I2CC Controller/Target Transmit/Receive Byte Buffer Register */
 #define AST2600_I2CC_STS_AND_BUFF		0x08
@@ -255,7 +256,7 @@ struct ast2600_i2c_bus {
 	struct i2c_timings	timing_info;
 	struct completion	cmd_complete;
 	struct i2c_msg		*msgs;
-	u8			*controller_dma_safe_buf;
+	u8			*dma_safe_buf;
 	dma_addr_t		controller_dma_addr;
 	u32			apb_clk;
 	u32			timeout;
@@ -271,6 +272,13 @@ struct ast2600_i2c_bus {
 	/* Buffer mode */
 	void __iomem		*buf_base;
 	struct i2c_smbus_alert_setup	alert_data;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	/* target structure */
+	int			target_operate;
+	unsigned char	*target_dma_buf;
+	dma_addr_t		target_dma_addr;
+	struct i2c_client	*target;
+#endif
 };
 
 static u32 ast2600_select_i2c_clock(struct ast2600_i2c_bus *i2c_bus)
@@ -361,6 +369,395 @@ static u8 ast2600_i2c_recover_bus(struct ast2600_i2c_bus *i2c_bus)
 	return ret;
 }
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static void ast2600_i2c_target_packet_dma_irq(struct ast2600_i2c_bus *i2c_bus, u32 sts)
+{
+	int target_rx_len = 0;
+	u32 cmd = 0;
+	u8 value;
+	int i;
+
+	sts &= ~(AST2600_I2CS_SLAVE_PENDING);
+	/* Handle i2c target timeout condition */
+	if (AST2600_I2CS_INACTIVE_TO & sts) {
+		cmd = TARGET_TRIGGER_CMD;
+		cmd |= AST2600_I2CS_RX_DMA_EN;
+		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_TARGET_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->target, 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->target, I2C_SLAVE_WRITE_REQUESTED, &value);
+		target_rx_len = AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
+						      AST2600_I2CS_DMA_LEN_STS));
+		for (i = 0; i < target_rx_len; i++) {
+			i2c_slave_event(i2c_bus->target, I2C_SLAVE_WRITE_RECEIVED,
+					&i2c_bus->target_dma_buf[i]);
+		}
+		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_TARGET_MSG_BUF_SIZE),
+		       i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
+		cmd = TARGET_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
+		break;
+	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_STOP:
+		i2c_slave_event(i2c_bus->target, I2C_SLAVE_STOP, &value);
+		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_TARGET_MSG_BUF_SIZE),
+		       i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
+		cmd = TARGET_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->target, I2C_SLAVE_WRITE_REQUESTED, &value);
+
+		target_rx_len = AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
+						      AST2600_I2CS_DMA_LEN_STS));
+		for (i = 0; i < target_rx_len; i++) {
+			i2c_slave_event(i2c_bus->target, I2C_SLAVE_WRITE_RECEIVED,
+					&i2c_bus->target_dma_buf[i]);
+		}
+		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_TARGET_MSG_BUF_SIZE),
+		       i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
+		if (sts & AST2600_I2CS_STOP)
+			i2c_slave_event(i2c_bus->target, I2C_SLAVE_STOP, &value);
+		cmd = TARGET_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->target, I2C_SLAVE_WRITE_REQUESTED, &value);
+
+		target_rx_len = AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
+						      AST2600_I2CS_DMA_LEN_STS));
+		for (i = 0; i < target_rx_len; i++) {
+			i2c_slave_event(i2c_bus->target, I2C_SLAVE_WRITE_RECEIVED,
+					&i2c_bus->target_dma_buf[i]);
+		}
+		i2c_slave_event(i2c_bus->target, I2C_SLAVE_READ_REQUESTED,
+				&i2c_bus->target_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 = TARGET_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->target, I2C_SLAVE_READ_REQUESTED,
+				&i2c_bus->target_dma_buf[0]);
+		writel(AST2600_I2CS_SET_TX_DMA_LEN(1),
+		       i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
+		cmd = TARGET_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->target, I2C_SLAVE_READ_PROCESSED,
+				&i2c_bus->target_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 = TARGET_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->target, I2C_SLAVE_STOP, &value);
+		writel(0, i2c_bus->reg_base + AST2600_I2CS_DMA_LEN_STS);
+		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_TARGET_MSG_BUF_SIZE),
+		       i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
+		cmd = TARGET_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
+		break;
+	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:
+		cmd = 0;
+		i2c_slave_event(i2c_bus->target, I2C_SLAVE_WRITE_REQUESTED, &value);
+		break;
+	case AST2600_I2CS_STOP:
+		cmd = 0;
+		i2c_slave_event(i2c_bus->target, I2C_SLAVE_STOP, &value);
+		break;
+	default:
+		dev_dbg(i2c_bus->dev, "unhandled target 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_target_packet_buff_irq(struct ast2600_i2c_bus *i2c_bus, u32 sts)
+{
+	int target_rx_len = 0;
+	u32 cmd = 0;
+	u8 value;
+	int i;
+
+	/* due to controller target is common buffer, need force the master stop not issue */
+	if (readl(i2c_bus->reg_base + AST2600_I2CM_CMD_STS) & GENMASK(15, 0)) {
+		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 target timeout condition */
+	if (AST2600_I2CS_INACTIVE_TO & sts) {
+		/* Reset timeout counter */
+		u32 ac_timing = readl(i2c_bus->reg_base + AST2600_I2CC_AC_TIMING) &
+				AST2600_I2CC_AC_TIMING_MASK;
+
+		writel(ac_timing, i2c_bus->reg_base + AST2600_I2CC_AC_TIMING);
+		ac_timing |= AST2600_I2CC_TTIMEOUT(i2c_bus->timeout);
+		writel(ac_timing, i2c_bus->reg_base + AST2600_I2CC_AC_TIMING);
+		writel(TARGET_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->target, I2C_SLAVE_STOP, &value);
+		i2c_bus->target_operate = 0;
+		return;
+	}
+
+	sts &= ~(AST2600_I2CS_PKT_DONE | AST2600_I2CS_PKT_ERROR);
+
+	if (sts & AST2600_I2CS_SLAVE_MATCH)
+		i2c_bus->target_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->target, 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->target, I2C_SLAVE_WRITE_REQUESTED, &value);
+		cmd = TARGET_TRIGGER_CMD;
+		if (sts & AST2600_I2CS_RX_DONE) {
+			target_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
+							       AST2600_I2CC_BUFF_CTRL));
+			for (i = 0; i < target_rx_len; i++) {
+				value = readb(i2c_bus->buf_base + 0x10 + i);
+				i2c_slave_event(i2c_bus->target, I2C_SLAVE_WRITE_RECEIVED, &value);
+			}
+		}
+		if (readl(i2c_bus->reg_base + AST2600_I2CS_CMD_STS) & AST2600_I2CS_RX_BUFF_EN)
+			cmd = 0;
+		else
+			cmd = TARGET_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 = TARGET_TRIGGER_CMD;
+		target_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
+						       AST2600_I2CC_BUFF_CTRL));
+		for (i = 0; i < target_rx_len; i++) {
+			value = readb(i2c_bus->buf_base + 0x10 + i);
+			i2c_slave_event(i2c_bus->target, 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 = TARGET_TRIGGER_CMD;
+		target_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
+								 AST2600_I2CC_BUFF_CTRL));
+		for (i = 0; i < target_rx_len; i++) {
+			value = readb(i2c_bus->buf_base + 0x10 + i);
+			i2c_slave_event(i2c_bus->target, I2C_SLAVE_WRITE_RECEIVED, &value);
+		}
+		i2c_slave_event(i2c_bus->target, 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 = TARGET_TRIGGER_CMD;
+		target_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
+								 AST2600_I2CC_BUFF_CTRL));
+		for (i = 0; i < target_rx_len; i++) {
+			value = readb(i2c_bus->buf_base + 0x10 + i);
+			i2c_slave_event(i2c_bus->target, 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->target, I2C_SLAVE_STOP, &value);
+		break;
+	case AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
+		cmd = TARGET_TRIGGER_CMD;
+		target_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
+								 AST2600_I2CC_BUFF_CTRL));
+		for (i = 0; i < target_rx_len; i++) {
+			value = readb(i2c_bus->buf_base + 0x10 + i);
+			i2c_slave_event(i2c_bus->target, 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->target, I2C_SLAVE_STOP, &value);
+		break;
+	case AST2600_I2CS_WAIT_TX_DMA | AST2600_I2CS_SLAVE_MATCH:
+		i2c_slave_event(i2c_bus->target, 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 = TARGET_TRIGGER_CMD | AST2600_I2CS_TX_BUFF_EN;
+		break;
+	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_WAIT_TX_DMA | AST2600_I2CS_RX_DONE:
+	case AST2600_I2CS_WAIT_TX_DMA | AST2600_I2CS_RX_DONE:
+	case AST2600_I2CS_WAIT_TX_DMA:
+		if (sts & AST2600_I2CS_SLAVE_MATCH)
+			i2c_slave_event(i2c_bus->target, I2C_SLAVE_WRITE_REQUESTED, &value);
+
+		if (sts & AST2600_I2CS_RX_DONE) {
+			target_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
+							AST2600_I2CC_BUFF_CTRL));
+			for (i = 0; i < target_rx_len; i++) {
+				value = readb(i2c_bus->buf_base + 0x10 + i);
+				i2c_slave_event(i2c_bus->target, I2C_SLAVE_WRITE_RECEIVED, &value);
+			}
+			i2c_slave_event(i2c_bus->target, I2C_SLAVE_READ_REQUESTED, &value);
+		} else {
+			i2c_slave_event(i2c_bus->target, 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 = TARGET_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->target, I2C_SLAVE_WRITE_REQUESTED, &value);
+		cmd = TARGET_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 = TARGET_TRIGGER_CMD;
+		i2c_slave_event(i2c_bus->target, I2C_SLAVE_STOP, &value);
+		break;
+	default:
+		dev_dbg(i2c_bus->dev, "unhandled target 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->target_operate = 0;
+}
+
+static void ast2600_i2c_target_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->target, 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->target, 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->target, 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->target, 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->target, 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_target_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;
+
+	/*
+	 * Target 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_target_packet_dma_irq(i2c_bus, isr);
+		else
+			ast2600_i2c_target_packet_buff_irq(i2c_bus, isr);
+	} else {
+		ast2600_i2c_target_byte_irq(i2c_bus, isr);
+	}
+
+	return 1;
+}
+#endif
+
 static int ast2600_i2c_setup_dma_tx(u32 cmd, struct ast2600_i2c_bus *i2c_bus)
 {
 	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
@@ -376,16 +773,16 @@ static int ast2600_i2c_setup_dma_tx(u32 cmd, struct ast2600_i2c_bus *i2c_bus)
 
 	if (cmd & AST2600_I2CM_START_CMD) {
 		cmd |= AST2600_I2CM_PKT_ADDR(msg->addr);
-		i2c_bus->controller_dma_safe_buf = i2c_get_dma_safe_msg_buf(msg, 1);
-		if (!i2c_bus->controller_dma_safe_buf)
+		i2c_bus->dma_safe_buf = i2c_get_dma_safe_msg_buf(msg, 1);
+		if (!i2c_bus->dma_safe_buf)
 			return -ENOMEM;
 		i2c_bus->controller_dma_addr =
-			dma_map_single(i2c_bus->dev, i2c_bus->controller_dma_safe_buf,
+			dma_map_single(i2c_bus->dev, i2c_bus->dma_safe_buf,
 				       msg->len, DMA_TO_DEVICE);
 		ret = dma_mapping_error(i2c_bus->dev, i2c_bus->controller_dma_addr);
 		if (ret) {
-			i2c_put_dma_safe_msg_buf(i2c_bus->controller_dma_safe_buf, msg, false);
-			i2c_bus->controller_dma_safe_buf = NULL;
+			i2c_put_dma_safe_msg_buf(i2c_bus->dma_safe_buf, msg, false);
+			i2c_bus->dma_safe_buf = NULL;
 			return ret;
 		}
 	}
@@ -502,16 +899,15 @@ static int ast2600_i2c_setup_dma_rx(struct ast2600_i2c_bus *i2c_bus)
 			cmd |= CONTROLLER_TRIGGER_LAST_STOP;
 	}
 	writel(AST2600_I2CM_SET_RX_DMA_LEN(xfer_len - 1), i2c_bus->reg_base + AST2600_I2CM_DMA_LEN);
-	i2c_bus->controller_dma_safe_buf = i2c_get_dma_safe_msg_buf(msg, 1);
-	if (!i2c_bus->controller_dma_safe_buf)
+	i2c_bus->dma_safe_buf = i2c_get_dma_safe_msg_buf(msg, 1);
+	if (!i2c_bus->dma_safe_buf)
 		return -ENOMEM;
 	i2c_bus->controller_dma_addr =
-		dma_map_single(i2c_bus->dev, i2c_bus->controller_dma_safe_buf,
-			       msg->len, DMA_FROM_DEVICE);
+		dma_map_single(i2c_bus->dev, i2c_bus->dma_safe_buf, msg->len, DMA_FROM_DEVICE);
 	ret = dma_mapping_error(i2c_bus->dev, i2c_bus->controller_dma_addr);
 	if (ret) {
-		i2c_put_dma_safe_msg_buf(i2c_bus->controller_dma_safe_buf, msg, false);
-		i2c_bus->controller_dma_safe_buf = NULL;
+		i2c_put_dma_safe_msg_buf(i2c_bus->dma_safe_buf, msg, false);
+		i2c_bus->dma_safe_buf = NULL;
 		return ret;
 	}
 	writel(i2c_bus->controller_dma_addr, i2c_bus->reg_base + AST2600_I2CM_RX_DMA);
@@ -659,9 +1055,8 @@ static void ast2600_i2c_controller_package_irq(struct ast2600_i2c_bus *i2c_bus,
 			if (i2c_bus->mode == DMA_MODE) {
 				dma_unmap_single(i2c_bus->dev, i2c_bus->controller_dma_addr,
 						 msg->len, DMA_TO_DEVICE);
-				i2c_put_dma_safe_msg_buf(i2c_bus->controller_dma_safe_buf,
-							 msg, true);
-				i2c_bus->controller_dma_safe_buf = NULL;
+				i2c_put_dma_safe_msg_buf(i2c_bus->dma_safe_buf, msg, true);
+				i2c_bus->dma_safe_buf = NULL;
 			}
 			i2c_bus->msgs_index++;
 			if (i2c_bus->msgs_index == i2c_bus->msgs_count) {
@@ -683,6 +1078,20 @@ static void ast2600_i2c_controller_package_irq(struct ast2600_i2c_bus *i2c_bus,
 		}
 		break;
 	case AST2600_I2CM_RX_DONE:
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+		/*
+		 * Workaround for controller/target package mode enable rx done stuck issue
+		 * When master go for first read (RX_DONE), target 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 target_cmd = readl(i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+
+			writel(0, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+			writel(target_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) {
@@ -712,9 +1121,8 @@ static void ast2600_i2c_controller_package_irq(struct ast2600_i2c_bus *i2c_bus,
 			if (i2c_bus->mode == DMA_MODE) {
 				dma_unmap_single(i2c_bus->dev, i2c_bus->controller_dma_addr,
 						 msg->len, DMA_FROM_DEVICE);
-				i2c_put_dma_safe_msg_buf(i2c_bus->controller_dma_safe_buf,
-							 msg, true);
-				i2c_bus->controller_dma_safe_buf = NULL;
+				i2c_put_dma_safe_msg_buf(i2c_bus->dma_safe_buf, msg, true);
+				i2c_bus->dma_safe_buf = NULL;
 			}
 
 			i2c_bus->msgs_index++;
@@ -812,6 +1220,12 @@ static irqreturn_t ast2600_i2c_bus_irq(int irq, void *dev_id)
 {
 	struct ast2600_i2c_bus *i2c_bus = dev_id;
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	if (readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL) & AST2600_I2CC_SLAVE_EN) {
+		if (ast2600_i2c_target_irq(i2c_bus))
+			return IRQ_HANDLED;
+	}
+#endif
 	return IRQ_RETVAL(ast2600_i2c_controller_irq(i2c_bus));
 }
 
@@ -828,12 +1242,30 @@ static int ast2600_i2c_controller_xfer(struct i2c_adapter *adap, struct i2c_msg
 			return ret;
 	}
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	if (i2c_bus->mode == BUFF_MODE) {
+		if (i2c_bus->target_operate)
+			return -EBUSY;
+		/* disable target isr */
+		writel(0, i2c_bus->reg_base + AST2600_I2CS_IER);
+		if (readl(i2c_bus->reg_base + AST2600_I2CS_ISR) || i2c_bus->target_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);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	/* avoid race condication target is wait and master wait 1st target operate */
+	if (i2c_bus->mode == BUFF_MODE)
+		writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_IER);
+#endif
 	if (ret)
 		goto controller_out;
 	timeout = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout);
@@ -850,7 +1282,26 @@ static int ast2600_i2c_controller_xfer(struct i2c_adapter *adap, struct i2c_msg
 		    (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) &
 		    AST2600_I2CC_BUS_BUSY_STS))
 			ast2600_i2c_recover_bus(i2c_bus);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+		if (ctrl & AST2600_I2CC_SLAVE_EN) {
+			u32 cmd = TARGET_TRIGGER_CMD;
 
+			if (i2c_bus->mode == DMA_MODE) {
+				cmd |= AST2600_I2CS_RX_DMA_EN;
+				writel(i2c_bus->target_dma_addr,
+				       i2c_bus->reg_base + AST2600_I2CS_RX_DMA);
+				writel(i2c_bus->target_dma_addr,
+				       i2c_bus->reg_base + AST2600_I2CS_TX_DMA);
+				writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_TARGET_MSG_BUF_SIZE),
+				       i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
+			} else if (i2c_bus->mode == BUFF_MODE) {
+				cmd = TARGET_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;
@@ -860,8 +1311,8 @@ static int ast2600_i2c_controller_xfer(struct i2c_adapter *adap, struct i2c_msg
 
 controller_out:
 	if (i2c_bus->mode == DMA_MODE) {
-		kfree(i2c_bus->controller_dma_safe_buf);
-		i2c_bus->controller_dma_safe_buf = NULL;
+		kfree(i2c_bus->dma_safe_buf);
+		i2c_bus->dma_safe_buf = NULL;
 	}
 
 	return ret;
@@ -889,7 +1340,78 @@ static void ast2600_i2c_init(struct ast2600_i2c_bus *i2c_bus)
 
 	/* Clear Interrupt */
 	writel(GENMASK(27, 0), i2c_bus->reg_base + AST2600_I2CM_ISR);
+
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	/* for memory buffer initial */
+	if (i2c_bus->mode == DMA_MODE) {
+		i2c_bus->target_dma_buf =
+			dmam_alloc_coherent(i2c_bus->dev, I2C_TARGET_MSG_BUF_SIZE,
+					    &i2c_bus->target_dma_addr, GFP_KERNEL);
+		if (!i2c_bus->target_dma_buf)
+			return;
+	}
+
+	writel(GENMASK(27, 0), i2c_bus->reg_base + AST2600_I2CS_ISR);
+
+	if (i2c_bus->mode == BYTE_MODE)
+		writel(GENMASK(15, 0), i2c_bus->reg_base + AST2600_I2CS_IER);
+	else
+		writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_IER);
+#endif
+}
+
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static int ast2600_i2c_reg_target(struct i2c_client *client)
+{
+	struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(client->adapter);
+	u32 cmd = TARGET_TRIGGER_CMD;
+
+	if (i2c_bus->target)
+		return -EINVAL;
+
+	dev_dbg(i2c_bus->dev, "target 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->target_dma_addr, i2c_bus->reg_base + AST2600_I2CS_RX_DMA);
+		writel(i2c_bus->target_dma_addr, i2c_bus->reg_base + AST2600_I2CS_TX_DMA);
+		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_TARGET_MSG_BUF_SIZE),
+		       i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
+	} else if (i2c_bus->mode == BUFF_MODE) {
+		cmd = TARGET_TRIGGER_CMD;
+	} else {
+		cmd &= ~AST2600_I2CS_PKT_MODE_EN;
+	}
+
+	writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+	i2c_bus->target = client;
+	/* Set target addr. */
+	writel(client->addr | AST2600_I2CS_ADDR1_ENABLE,
+	       i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
+
+	return 0;
+}
+
+static int ast2600_i2c_unreg_target(struct i2c_client *client)
+{
+	struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(client->adapter);
+
+	/* Turn off target 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->target = NULL;
+
+	return 0;
 }
+#endif
 
 static u32 ast2600_i2c_functionality(struct i2c_adapter *adap)
 {
@@ -899,6 +1421,10 @@ static u32 ast2600_i2c_functionality(struct i2c_adapter *adap)
 static const struct i2c_algorithm i2c_ast2600_algorithm = {
 	.xfer = ast2600_i2c_controller_xfer,
 	.functionality = ast2600_i2c_functionality,
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	.reg_target = ast2600_i2c_reg_target,
+	.unreg_target = ast2600_i2c_unreg_target,
+#endif
 };
 
 static int ast2600_i2c_probe(struct platform_device *pdev)
@@ -934,6 +1460,9 @@ static int ast2600_i2c_probe(struct platform_device *pdev)
 		regmap_write(i2c_bus->global_regs, AST2600_I2CG_CLK_DIV_CTRL, I2CCG_DIV_CTRL);
 	}
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	i2c_bus->target_operate = 0;
+#endif
 	i2c_bus->dev = dev;
 	i2c_bus->mode = BUFF_MODE;
 
-- 
2.34.1

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Rob Herring (Arm)]

On Mon, 24 Feb 2025 13:59:34 +0800, Ryan Chen wrote:
> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma
> and description for ast2600-i2cv2.
> 
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml:82:1: [error] duplication of key "allOf" in mapping (key-duplicates)

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml: ignoring, error parsing file
./Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml:82:1: found duplicate key "allOf" with value "[]" (original value: "[]")
make[2]: *** Deleting file 'Documentation/devicetree/bindings/i2c/aspeed,i2c.example.dts'
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml:82:1: found duplicate key "allOf" with value "[]" (original value: "[]")
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/i2c/aspeed,i2c.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1511: dt_binding_check] Error 2
make: *** [Makefile:251: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250224055936.1804279-2-ryan_chen@aspeedtech.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new register mode driver

[Philipp Zabel]

On Mo, 2025-02-24 at 13:59 +0800, Ryan Chen wrote:
> 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 new register mode have separate register
> set to control i2c controller and target. This patch is for i2c
> controller mode driver.
> 
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
>  drivers/i2c/busses/Kconfig       |   11 +
>  drivers/i2c/busses/Makefile      |    1 +
>  drivers/i2c/busses/i2c-ast2600.c | 1036 ++++++++++++++++++++++++++++++
>  3 files changed, 1048 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-ast2600.c
> 
[...]
> diff --git a/drivers/i2c/busses/i2c-ast2600.c b/drivers/i2c/busses/i2c-ast2600.c
> new file mode 100644
> index 000000000000..bfac507693dd
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ast2600.c
> @@ -0,0 +1,1036 @@
[...]
> +static int ast2600_i2c_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ast2600_i2c_bus *i2c_bus;
> +	struct resource *res;
> +	u32 global_ctrl;
> +	int ret;
> +
> +	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);

No reset_control_assert() in the error paths below? You could get that
and simplify this by using devm_reset_control_get_shared_deasserted().

regards
Philipp

RE: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new register mode driver

[Ryan Chen]

> Subject: Re: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new register
> mode driver
> 
> On Mo, 2025-02-24 at 13:59 +0800, Ryan Chen wrote:
> > 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 new register mode
> > have separate register set to control i2c controller and target. This
> > patch is for i2c controller mode driver.
> >
> > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> > ---
> >  drivers/i2c/busses/Kconfig       |   11 +
> >  drivers/i2c/busses/Makefile      |    1 +
> >  drivers/i2c/busses/i2c-ast2600.c | 1036
> > ++++++++++++++++++++++++++++++
> >  3 files changed, 1048 insertions(+)
> >  create mode 100644 drivers/i2c/busses/i2c-ast2600.c
> >
> [...]
> > diff --git a/drivers/i2c/busses/i2c-ast2600.c
> > b/drivers/i2c/busses/i2c-ast2600.c
> > new file mode 100644
> > index 000000000000..bfac507693dd
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-ast2600.c
> > @@ -0,0 +1,1036 @@
> [...]
> > +static int ast2600_i2c_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct ast2600_i2c_bus *i2c_bus;
> > +	struct resource *res;
> > +	u32 global_ctrl;
> > +	int ret;
> > +
> > +	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);
> 
> No reset_control_assert() in the error paths below? You could get that and
> simplify this by using devm_reset_control_get_shared_deasserted().
> 
Sorry, devm_reset_control_get_shared_deasserted is new for me.
Do you mean modify by following 

i2c_bus->rst = devm_reset_control_get_shared_deasserted(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);

> regards
> Philipp

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On Mon, Feb 24, 2025 at 01:59:34PM +0800, Ryan Chen wrote:
> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma
> and description for ast2600-i2cv2.
> 
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> index 5b9bd2feda3b..356033d18f90 100644
> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> @@ -44,12 +44,60 @@ properties:
>      description: frequency of the bus clock in Hz defaults to 100 kHz when not
>        specified
>  
> +  multi-master:
> +    type: boolean
> +    description:
> +      states that there is another master active on this bus

Except that this wasn't ever tested...

Don't duplicate properties. i2c-controller schema has it already.

> +
> +  aspeed,enable-dma:
> +    type: boolean
> +    description: |
> +      I2C bus enable dma mode transfer.
> +
> +      ASPEED ast2600 platform equipped with 16 I2C controllers that share a
> +      single DMA engine. DTS files can specify the data transfer mode to/from
> +      the device, either DMA or programmed I/O. However, hardware limitations

so what is byte mode?

> +      may require a DTS to manually allocate which controller can use DMA mode.
> +      The "aspeed,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
> +      enable DMA mode for that one controller.
> +
> +  aspeed,enable-byte:
> +    type: boolean
> +    description: |
> +      I2C bus enable byte mode transfer.

No, either this is expressed as lack of dma mode property or if you have
three modes, then rather some enum (aspeed,transfer-mode ?)



> +
> +  aspeed,global-regs:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: The phandle of i2c global register node.

For what? Same question as usual: do not repeat property name, but say
what is this used for and what exactly it points to.

s/i2c/I2C/ but then what is "I2C global register node"? This is how you
call your device in datasheet?


> +
>  required:
>    - reg
>    - compatible
>    - clocks
>    - resets
>  
> +allOf:
> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: aspeed,ast2600-i2cv2

NAK, undocumented compatible.

> +
> +    then:
> +      properties:
> +        reg:
> +          minItems: 2


Best regards,
Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 24/02/2025 10:11, Krzysztof Kozlowski wrote:
> 
> 
>> +
>>  required:
>>    - reg
>>    - compatible
>>    - clocks
>>    - resets
>>  
>> +allOf:
>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: aspeed,ast2600-i2cv2
> 
> NAK, undocumented compatible.

Heh, and this was in previous versions as well.

You have never tested your DTS. At v16 still no testing. This is not
acceptable.

Best regards,
Krzysztof

Re: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new register mode driver

[Philipp Zabel]

On Mo, 2025-02-24 at 09:04 +0000, Ryan Chen wrote:
> > Subject: Re: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new
> > register
> > mode driver
> > 
> > On Mo, 2025-02-24 at 13:59 +0800, Ryan Chen wrote:
> > > 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 new register
> > > mode
> > > have separate register set to control i2c controller and target.
> > > This
> > > patch is for i2c controller mode driver.
> > > 
> > > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> > > ---
> > >  drivers/i2c/busses/Kconfig       |   11 +
> > >  drivers/i2c/busses/Makefile      |    1 +
> > >  drivers/i2c/busses/i2c-ast2600.c | 1036
> > > ++++++++++++++++++++++++++++++
> > >  3 files changed, 1048 insertions(+)
> > >  create mode 100644 drivers/i2c/busses/i2c-ast2600.c
> > > 
> > [...]
> > > diff --git a/drivers/i2c/busses/i2c-ast2600.c
> > > b/drivers/i2c/busses/i2c-ast2600.c
> > > new file mode 100644
> > > index 000000000000..bfac507693dd
> > > --- /dev/null
> > > +++ b/drivers/i2c/busses/i2c-ast2600.c
> > > @@ -0,0 +1,1036 @@
> > [...]
> > > +static int ast2600_i2c_probe(struct platform_device *pdev) {
> > > +	struct device *dev = &pdev->dev;
> > > +	struct ast2600_i2c_bus *i2c_bus;
> > > +	struct resource *res;
> > > +	u32 global_ctrl;
> > > +	int ret;
> > > +
> > > +	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);
> > 
> > No reset_control_assert() in the error paths below? You could get
> > that and
> > simplify this by using devm_reset_control_get_shared_deasserted().
> > 
> Sorry, devm_reset_control_get_shared_deasserted is new for me.
> Do you mean modify by following 
> 
> i2c_bus->rst = devm_reset_control_get_shared_deasserted(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);

Yes see [1]. Remove the reset_control_assert() in ast2600_i2c_remove()
as well.

[1] https://docs.kernel.org/driver-api/reset.html#c.devm_reset_control_get_shared_deasserted

regards
Philipp

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On Mon, Feb 24, 2025 at 01:59:34PM +0800, Ryan Chen wrote:
> > Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma
> > and description for ast2600-i2cv2.
> >
> > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> > ---
> >  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 58 +++++++++++++++++++
> >  1 file changed, 58 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > index 5b9bd2feda3b..356033d18f90 100644
> > --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > @@ -44,12 +44,60 @@ properties:
> >      description: frequency of the bus clock in Hz defaults to 100 kHz when
> not
> >        specified
> >
> > +  multi-master:
> > +    type: boolean
> > +    description:
> > +      states that there is another master active on this bus
> 
> Except that this wasn't ever tested...
> 
> Don't duplicate properties. i2c-controller schema has it already.

I will remove it to avoid duplication.
> 
> > +
> > +  aspeed,enable-dma:
> > +    type: boolean
> > +    description: |
> > +      I2C bus enable dma mode transfer.
> > +
> > +      ASPEED ast2600 platform equipped with 16 I2C controllers that
> share a
> > +      single DMA engine. DTS files can specify the data transfer mode
> to/from
> > +      the device, either DMA or programmed I/O. However, hardware
> > + limitations
> 
> so what is byte mode?
I explain in cover, I will add here also. 
aspeed,enable-byte:
Force i2c controller use byte mode transfer. the byte mode transfer
will send i2c data each byte by byte, inlcude address read/write.

> 
> > +      may require a DTS to manually allocate which controller can use
> DMA mode.
> > +      The "aspeed,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
> > +      enable DMA mode for that one controller.
> > +
> > +  aspeed,enable-byte:
> > +    type: boolean
> > +    description: |
> > +      I2C bus enable byte mode transfer.
> 
> No, either this is expressed as lack of dma mode property or if you have three
> modes, then rather some enum (aspeed,transfer-mode ?)

Thanks suggestion, I will using an enum property like aspeed,transfer-mode instead.
> 
> 
> 
> > +
> > +  aspeed,global-regs:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: The phandle of i2c global register node.
> 
> For what? Same question as usual: do not repeat property name, but say what
> is this used for and what exactly it points to.
> 
> s/i2c/I2C/ but then what is "I2C global register node"? This is how you call your
> device in datasheet?
> 
I do descript in cover, should add into the yaml file ?

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

> 
> > +
> >  required:
> >    - reg
> >    - compatible
> >    - clocks
> >    - resets
> >
> > +allOf:
> > +  - $ref: /schemas/i2c/i2c-controller.yaml#
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: aspeed,ast2600-i2cv2
> 
> NAK, undocumented compatible.

Sorry, I should add what kind of document about this compatible?

The cover descripts it, should I add into yaml?


This series add AST2600 i2cv2 new register set driver. The i2cv2 driver
is new register set that have new clock divider option for more flexiable
generation. And also have separate i2c controller and target register
set for control, patch #2 is i2c controller driver only, patch #3 is add
i2c target mode driver.

The legacy register layout is mix controller/target register control
together. The following is add more detail description about new register
layout. And new feature set add for register.
> 
> > +
> > +    then:
> > +      properties:
> > +        reg:
> > +          minItems: 2
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 26/02/2025 10:28, Ryan Chen wrote:
>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>> AST2600-i2cv2
>>
>> On Mon, Feb 24, 2025 at 01:59:34PM +0800, Ryan Chen wrote:
>>> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma
>>> and description for ast2600-i2cv2.
>>>
>>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
>>> ---
>>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 58 +++++++++++++++++++
>>>  1 file changed, 58 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> index 5b9bd2feda3b..356033d18f90 100644
>>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> @@ -44,12 +44,60 @@ properties:
>>>      description: frequency of the bus clock in Hz defaults to 100 kHz when
>> not
>>>        specified
>>>
>>> +  multi-master:
>>> +    type: boolean
>>> +    description:
>>> +      states that there is another master active on this bus
>>
>> Except that this wasn't ever tested...
>>
>> Don't duplicate properties. i2c-controller schema has it already.
> 
> I will remove it to avoid duplication.
>>
>>> +
>>> +  aspeed,enable-dma:
>>> +    type: boolean
>>> +    description: |
>>> +      I2C bus enable dma mode transfer.
>>> +
>>> +      ASPEED ast2600 platform equipped with 16 I2C controllers that
>> share a
>>> +      single DMA engine. DTS files can specify the data transfer mode
>> to/from
>>> +      the device, either DMA or programmed I/O. However, hardware
>>> + limitations
>>
>> so what is byte mode?
> I explain in cover, I will add here also. 

Cover letters do not get merged and we do not read them, except when
looking for dependencies and explanations of process (like RFC). Your
hardware description must be fully contained in commits, not cover letter.


> aspeed,enable-byte:
> Force i2c controller use byte mode transfer. the byte mode transfer
> will send i2c data each byte by byte, inlcude address read/write.

Isn't this standard FIFO mode?

Why anyone would need to enable byte mode for given board?

> 
>>
>>> +      may require a DTS to manually allocate which controller can use
>> DMA mode.
>>> +      The "aspeed,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
>>> +      enable DMA mode for that one controller.
>>> +
>>> +  aspeed,enable-byte:
>>> +    type: boolean
>>> +    description: |
>>> +      I2C bus enable byte mode transfer.
>>
>> No, either this is expressed as lack of dma mode property or if you have three
>> modes, then rather some enum (aspeed,transfer-mode ?)
> 
> Thanks suggestion, I will using an enum property like aspeed,transfer-mode instead.
>>
>>
>>
>>> +
>>> +  aspeed,global-regs:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: The phandle of i2c global register node.
>>
>> For what? Same question as usual: do not repeat property name, but say what
>> is this used for and what exactly it points to.
>>
>> s/i2c/I2C/ but then what is "I2C global register node"? This is how you call your
>> device in datasheet?
>>
> I do descript in cover, should add into the yaml file ?


Again, cover letter does not matter. Your hardware must be explained here.

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

So this means you implement clock controller via syscon?

> 
>>
>>> +
>>>  required:
>>>    - reg
>>>    - compatible
>>>    - clocks
>>>    - resets
>>>
>>> +allOf:
>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: aspeed,ast2600-i2cv2
>>
>> NAK, undocumented compatible.
> 
> Sorry, I should add what kind of document about this compatible?

You cannot add new compatibles without documenting them. Documentation
is in the form of DT schema and each compatible must be listed (in some
way) in compatible property description.


Best regards,
Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 26/02/2025 10:28, Ryan Chen wrote:
> >> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On Mon, Feb 24, 2025 at 01:59:34PM +0800, Ryan Chen wrote:
> >>> Add ast2600-i2cv2 compatible and aspeed,global-regs,
> >>> aspeed,enable-dma and description for ast2600-i2cv2.
> >>>
> >>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> >>> ---
> >>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 58
> +++++++++++++++++++
> >>>  1 file changed, 58 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> index 5b9bd2feda3b..356033d18f90 100644
> >>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> @@ -44,12 +44,60 @@ properties:
> >>>      description: frequency of the bus clock in Hz defaults to 100
> >>> kHz when
> >> not
> >>>        specified
> >>>
> >>> +  multi-master:
> >>> +    type: boolean
> >>> +    description:
> >>> +      states that there is another master active on this bus
> >>
> >> Except that this wasn't ever tested...
> >>
> >> Don't duplicate properties. i2c-controller schema has it already.
> >
> > I will remove it to avoid duplication.
> >>
> >>> +
> >>> +  aspeed,enable-dma:
> >>> +    type: boolean
> >>> +    description: |
> >>> +      I2C bus enable dma mode transfer.
> >>> +
> >>> +      ASPEED ast2600 platform equipped with 16 I2C controllers that
> >> share a
> >>> +      single DMA engine. DTS files can specify the data transfer
> >>> + mode
> >> to/from
> >>> +      the device, either DMA or programmed I/O. However, hardware
> >>> + limitations
> >>
> >> so what is byte mode?
> > I explain in cover, I will add here also.
> 
> Cover letters do not get merged and we do not read them, except when looking
> for dependencies and explanations of process (like RFC). Your hardware
> description must be fully contained in commits, not cover letter.

Got it, I will paste it in commits. 
> 
> 
> > aspeed,enable-byte:
> > Force i2c controller use byte mode transfer. the byte mode transfer
> > will send i2c data each byte by byte, inlcude address read/write.
> 
> Isn't this standard FIFO mode?
Yes, it is.
> 
> Why anyone would need to enable byte mode for given board?
By default, it is buffer-mode, for performance, I don't want user enable byte-mode, it will increase CPU utilize.
But someone want to be force enable byte-mode, so I add properties. 
https://patchwork.ozlabs.org/project/linux-aspeed/patch/20241007035235.2254138-3-ryan_chen@aspeedtech.com/
> 
> >
> >>
> >>> +      may require a DTS to manually allocate which controller can
> >>> + use
> >> DMA mode.
> >>> +      The "aspeed,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
> >>> +      enable DMA mode for that one controller.
> >>> +
> >>> +  aspeed,enable-byte:
> >>> +    type: boolean
> >>> +    description: |
> >>> +      I2C bus enable byte mode transfer.
> >>
> >> No, either this is expressed as lack of dma mode property or if you
> >> have three modes, then rather some enum (aspeed,transfer-mode ?)
> >
> > Thanks suggestion, I will using an enum property like aspeed,transfer-mode
> instead.
> >>
> >>
> >>
> >>> +
> >>> +  aspeed,global-regs:
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>> +    description: The phandle of i2c global register node.
> >>
> >> For what? Same question as usual: do not repeat property name, but
> >> say what is this used for and what exactly it points to.
> >>
> >> s/i2c/I2C/ but then what is "I2C global register node"? This is how
> >> you call your device in datasheet?
> >>
> > I do descript in cover, should add into the yaml file ?
> 
> 
> Again, cover letter does not matter. Your hardware must be explained here.
Will add into commit. 
> 
> >
> > aspeed,global-regs:
> > This global register is needed, global register is setting for new
> > clock divide control, and new register set control.
> 
> So this means you implement clock controller via syscon?
No, it is just mode switch. It also explain in cover. I will add it in commit. 
The legacy register layout is mix controller/target register control together. The following is add more detail description about new register layout. And new feature set add for register.
> 
> >
> >>
> >>> +
> >>>  required:
> >>>    - reg
> >>>    - compatible
> >>>    - clocks
> >>>    - resets
> >>>
> >>> +allOf:
> >>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            const: aspeed,ast2600-i2cv2
> >>
> >> NAK, undocumented compatible.
> >
> > Sorry, I should add what kind of document about this compatible?
> 
> You cannot add new compatibles without documenting them. Documentation
> is in the form of DT schema and each compatible must be listed (in some
> way) in compatible property description.

Sorry, do you mean, I add following in yaml or commit message?

This series add AST2600 i2cv2 new register set driver. The i2cv2 driver is new register set that have new clock divider option for more flexiable generation. And also have separate i2c controller and target register set for control, patch #2 is i2c controller driver only, patch #3 is add i2c target mode driver.

The legacy register layout is mix controller/target 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 controller and target mode control.
-Support 4 individual DMA buffers for controller Tx and Rx, target Tx and Rx.

> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 27/02/2025 09:19, Ryan Chen wrote:
>>
>>
>>> aspeed,enable-byte:
>>> Force i2c controller use byte mode transfer. the byte mode transfer
>>> will send i2c data each byte by byte, inlcude address read/write.
>>
>> Isn't this standard FIFO mode?
> Yes, it is.
>>
>> Why anyone would need to enable byte mode for given board?
> By default, it is buffer-mode, for performance, I don't want user enable byte-mode, it will increase CPU utilize.
> But someone want to be force enable byte-mode, so I add properties. 
> https://patchwork.ozlabs.org/project/linux-aspeed/patch/20241007035235.2254138-3-ryan_chen@aspeedtech.com/


I don't see the reason why this would be a board property.

I understood need for DMA because it is shared and only some of the
controllers can use it. But why choice between buffer and FIFO depending
on hardware?


>>
>>>
>>>>
>>>>> +      may require a DTS to manually allocate which controller can
>>>>> + use
>>>> DMA mode.
>>>>> +      The "aspeed,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
>>>>> +      enable DMA mode for that one controller.
>>>>> +
>>>>> +  aspeed,enable-byte:
>>>>> +    type: boolean
>>>>> +    description: |
>>>>> +      I2C bus enable byte mode transfer.
>>>>
>>>> No, either this is expressed as lack of dma mode property or if you
>>>> have three modes, then rather some enum (aspeed,transfer-mode ?)
>>>
>>> Thanks suggestion, I will using an enum property like aspeed,transfer-mode
>> instead.
>>>>
>>>>
>>>>
>>>>> +
>>>>> +  aspeed,global-regs:
>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>>> +    description: The phandle of i2c global register node.
>>>>
>>>> For what? Same question as usual: do not repeat property name, but
>>>> say what is this used for and what exactly it points to.
>>>>
>>>> s/i2c/I2C/ but then what is "I2C global register node"? This is how
>>>> you call your device in datasheet?
>>>>
>>> I do descript in cover, should add into the yaml file ?
>>
>>
>> Again, cover letter does not matter. Your hardware must be explained here.
> Will add into commit. 
>>
>>>
>>> aspeed,global-regs:
>>> This global register is needed, global register is setting for new
>>> clock divide control, and new register set control.
>>
>> So this means you implement clock controller via syscon?
> No, it is just mode switch. It also explain in cover. I will add it in commit. 
> The legacy register layout is mix controller/target register control together. The following is add more detail description about new register layout. And new feature set add for register.
>>
>>>
>>>>
>>>>> +
>>>>>  required:
>>>>>    - reg
>>>>>    - compatible
>>>>>    - clocks
>>>>>    - resets
>>>>>
>>>>> +allOf:
>>>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>>>>> +  - if:
>>>>> +      properties:
>>>>> +        compatible:
>>>>> +          contains:
>>>>> +            const: aspeed,ast2600-i2cv2
>>>>
>>>> NAK, undocumented compatible.
>>>
>>> Sorry, I should add what kind of document about this compatible?
>>
>> You cannot add new compatibles without documenting them. Documentation
>> is in the form of DT schema and each compatible must be listed (in some
>> way) in compatible property description.
> 
> Sorry, do you mean, I add following in yaml or commit message?

You need to list this in compatibles first.

> 
> This series add AST2600 i2cv2 new register set driver. The i2cv2 driver is new register set that have new clock divider option for more flexiable generation. And also have separate i2c controller and target register set for control, patch #2 is i2c controller driver only, patch #3 is add i2c target mode driver.

All this describes driver, not hardware.

> 
> The legacy register layout is mix controller/target 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 controller and target mode control.
> -Support 4 individual DMA buffers for controller Tx and Rx, target Tx and Rx.

Does it mean hardware changed on AST2600? Or these are different devices
than aspeed,ast2600-i2c-bus? If this is not a different device, how one
SoC can have two different flavors of same device in the same instance?




Best regards,
Krzysztof

Re: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new register mode driver

[kernel test robot]

Hi Ryan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on andi-shyti/i2c/i2c-host]
[also build test WARNING on linus/master v6.14-rc4 next-20250227]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ryan-Chen/dt-bindings-i2c-aspeed-support-for-AST2600-i2cv2/20250224-140221
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20250224055936.1804279-3-ryan_chen%40aspeedtech.com
patch subject: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new register mode driver
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20250228/202502280902.U0gLDhve-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250228/202502280902.U0gLDhve-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502280902.U0gLDhve-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/i2c/busses/i2c-ast2600.c: In function 'ast2600_i2c_recover_bus':
>> drivers/i2c/busses/i2c-ast2600.c:345:32: warning: unsigned conversion from 'int' to 'u8' {aka 'unsigned char'} changes value from '-145' to '111' [-Woverflow]
     345 |                         return -ETIMEDOUT;
         |                                ^


vim +345 drivers/i2c/busses/i2c-ast2600.c

   315	
   316	static u8 ast2600_i2c_recover_bus(struct ast2600_i2c_bus *i2c_bus)
   317	{
   318		u32 state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
   319		int ret = 0;
   320		u32 ctrl;
   321		int r;
   322	
   323		dev_dbg(i2c_bus->dev, "%d-bus recovery bus [%x]\n", i2c_bus->adap.nr, state);
   324	
   325		ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
   326	
   327		/* Disable controller */
   328		writel(ctrl & ~(AST2600_I2CC_MASTER_EN | AST2600_I2CC_SLAVE_EN),
   329		       i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
   330	
   331		writel(readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL) | AST2600_I2CC_MASTER_EN,
   332		       i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
   333	
   334		reinit_completion(&i2c_bus->cmd_complete);
   335		i2c_bus->cmd_err = 0;
   336	
   337		/* Check 0x14's SDA and SCL status */
   338		state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
   339		if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & AST2600_I2CC_SCL_LINE_STS)) {
   340			writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
   341			r = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout);
   342			if (r == 0) {
   343				dev_dbg(i2c_bus->dev, "recovery timed out\n");
   344				writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
 > 345				return -ETIMEDOUT;
   346			} else if (i2c_bus->cmd_err) {
   347				dev_dbg(i2c_bus->dev, "recovery error\n");
   348				ret = -EPROTO;
   349			}
   350		}
   351	
   352		/* Recovery done */
   353		state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
   354		if (state & AST2600_I2CC_BUS_BUSY_STS) {
   355			dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n", state);
   356			ret = -EPROTO;
   357		}
   358	
   359		/* restore original controller setting */
   360		writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
   361		return ret;
   362	}
   363	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new register mode driver

[Andy Shevchenko]

On Fri, Feb 28, 2025 at 09:28:59AM +0800, kernel test robot wrote:
> Hi Ryan,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on andi-shyti/i2c/i2c-host]
> [also build test WARNING on linus/master v6.14-rc4 next-20250227]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Ryan-Chen/dt-bindings-i2c-aspeed-support-for-AST2600-i2cv2/20250224-140221
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
> patch link:    https://lore.kernel.org/r/20250224055936.1804279-3-ryan_chen%40aspeedtech.com
> patch subject: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new register mode driver
> config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20250228/202502280902.U0gLDhve-lkp@intel.com/config)
> compiler: mips-linux-gcc (GCC) 14.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250228/202502280902.U0gLDhve-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202502280902.U0gLDhve-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):

My gosh, this is valid report. But it looks like a preexisted issue.
Can somebody fix this, please?

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 27/02/2025 09:19, Ryan Chen wrote:
> >>
> >>
> >>> aspeed,enable-byte:
> >>> Force i2c controller use byte mode transfer. the byte mode transfer
> >>> will send i2c data each byte by byte, inlcude address read/write.
> >>
> >> Isn't this standard FIFO mode?
> > Yes, it is.
> >>
> >> Why anyone would need to enable byte mode for given board?
> > By default, it is buffer-mode, for performance, I don't want user enable
> byte-mode, it will increase CPU utilize.
> > But someone want to be force enable byte-mode, so I add properties.
> > https://patchwork.ozlabs.org/project/linux-aspeed/patch/20241007035235
> > .2254138-3-ryan_chen@aspeedtech.com/
> 
> 
> I don't see the reason why this would be a board property.
> 
> I understood need for DMA because it is shared and only some of the
> controllers can use it. But why choice between buffer and FIFO depending on
> hardware?

By default, the I2C controller runs in buffer mode. Each i2c bus have 16bytes buffer to transmit data.
Enabling byte mode will increases CPU utilization, so it is not recommended. 
However, some user might require forcing byte mode, so I added this property to allow that.

> 
> 
> >>
> >>>
> >>>>
> >>>>> +      may require a DTS to manually allocate which controller can
> >>>>> + use
> >>>> DMA mode.
> >>>>> +      The "aspeed,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
> >>>>> +      enable DMA mode for that one controller.
> >>>>> +
> >>>>> +  aspeed,enable-byte:
> >>>>> +    type: boolean
> >>>>> +    description: |
> >>>>> +      I2C bus enable byte mode transfer.
> >>>>
> >>>> No, either this is expressed as lack of dma mode property or if you
> >>>> have three modes, then rather some enum (aspeed,transfer-mode ?)
> >>>
> >>> Thanks suggestion, I will using an enum property like
> >>> aspeed,transfer-mode
> >> instead.
> >>>>
> >>>>
> >>>>
> >>>>> +
> >>>>> +  aspeed,global-regs:
> >>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>>> +    description: The phandle of i2c global register node.
> >>>>
> >>>> For what? Same question as usual: do not repeat property name, but
> >>>> say what is this used for and what exactly it points to.
> >>>>
> >>>> s/i2c/I2C/ but then what is "I2C global register node"? This is how
> >>>> you call your device in datasheet?
> >>>>
> >>> I do descript in cover, should add into the yaml file ?
> >>
> >>
> >> Again, cover letter does not matter. Your hardware must be explained here.
> > Will add into commit.
> >>
> >>>
> >>> aspeed,global-regs:
> >>> This global register is needed, global register is setting for new
> >>> clock divide control, and new register set control.
> >>
> >> So this means you implement clock controller via syscon?
> > No, it is just mode switch. It also explain in cover. I will add it in commit.
> > The legacy register layout is mix controller/target register control together.
> The following is add more detail description about new register layout. And
> new feature set add for register.
> >>
> >>>
> >>>>
> >>>>> +
> >>>>>  required:
> >>>>>    - reg
> >>>>>    - compatible
> >>>>>    - clocks
> >>>>>    - resets
> >>>>>
> >>>>> +allOf:
> >>>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> >>>>> +  - if:
> >>>>> +      properties:
> >>>>> +        compatible:
> >>>>> +          contains:
> >>>>> +            const: aspeed,ast2600-i2cv2
> >>>>
> >>>> NAK, undocumented compatible.
> >>>
> >>> Sorry, I should add what kind of document about this compatible?
> >>
> >> You cannot add new compatibles without documenting them.
> >> Documentation is in the form of DT schema and each compatible must be
> >> listed (in some
> >> way) in compatible property description.
> >
> > Sorry, do you mean, I add following in yaml or commit message?
> 
> You need to list this in compatibles first.

I will add it in compatible in next submit.

	enum:
		-aspeed,ast2600-i2cv2
> 
> >
> > This series add AST2600 i2cv2 new register set driver. The i2cv2 driver is new
> register set that have new clock divider option for more flexiable generation.
> And also have separate i2c controller and target register set for control, patch
> #2 is i2c controller driver only, patch #3 is add i2c target mode driver.
> 
> All this describes driver, not hardware.

Sorry, the cover letter description the register. I will add int in commit message.

-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 controller and target mode control.
-Support 4 individual DMA buffers for controller Tx and Rx,
target 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 controller DMA command mode.
-Bus auto timeout for new controller/target 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

> 
> >
> > The legacy register layout is mix controller/target 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 controller and target mode control.
> > -Support 4 individual DMA buffers for controller Tx and Rx, target Tx and Rx.
> 
> Does it mean hardware changed on AST2600? 
No Hw change, it is different mode setting will have another mode register setting.
Mode setting is in global register, I will add in next commit message

I2CG00: Device Master Mode Interrupt Status Register (I2CG0C[3]=1)
I2CG00: Device Master/Slave Mode Interrupt Status Register (I2CG0C[3]=0)
I2CG04: Device Slave Mode Interrupt Status Register
I2CG0C: Global Control Register
I2CG10: New Clock Divider Control Register (I2CG0C[1] = 1)

> Or these are different devices
> than aspeed,ast2600-i2c-bus? If this is not a different device, how one SoC can
> have two different flavors of same device in the same instance?

When global setting for new, will new register mapping, no setting will keep old register mapping.
> 
> 
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 05/03/2025 10:35, Ryan Chen wrote:
>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>> AST2600-i2cv2
>>
>> On 27/02/2025 09:19, Ryan Chen wrote:
>>>>
>>>>
>>>>> aspeed,enable-byte:
>>>>> Force i2c controller use byte mode transfer. the byte mode transfer
>>>>> will send i2c data each byte by byte, inlcude address read/write.
>>>>
>>>> Isn't this standard FIFO mode?
>>> Yes, it is.
>>>>
>>>> Why anyone would need to enable byte mode for given board?
>>> By default, it is buffer-mode, for performance, I don't want user enable
>> byte-mode, it will increase CPU utilize.
>>> But someone want to be force enable byte-mode, so I add properties.
>>> https://patchwork.ozlabs.org/project/linux-aspeed/patch/20241007035235
>>> .2254138-3-ryan_chen@aspeedtech.com/
>>
>>
>> I don't see the reason why this would be a board property.
>>
>> I understood need for DMA because it is shared and only some of the
>> controllers can use it. But why choice between buffer and FIFO depending on
>> hardware?
> 
> By default, the I2C controller runs in buffer mode. Each i2c bus have 16bytes buffer to transmit data.
> Enabling byte mode will increases CPU utilization, so it is not recommended. 
> However, some user might require forcing byte mode, so I added this property to allow that.


You are not answering the question. I asked why the choice depends on
hardware and you answer "user might required".

Do you understand that this is about hardware, not user choices?


>>
>>
>>>>
>>>>>
>>>>>>
>>>>>>> +      may require a DTS to manually allocate which controller can
>>>>>>> + use
>>>>>> DMA mode.
>>>>>>> +      The "aspeed,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
>>>>>>> +      enable DMA mode for that one controller.
>>>>>>> +
>>>>>>> +  aspeed,enable-byte:
>>>>>>> +    type: boolean
>>>>>>> +    description: |
>>>>>>> +      I2C bus enable byte mode transfer.
>>>>>>
>>>>>> No, either this is expressed as lack of dma mode property or if you
>>>>>> have three modes, then rather some enum (aspeed,transfer-mode ?)
>>>>>
>>>>> Thanks suggestion, I will using an enum property like
>>>>> aspeed,transfer-mode
>>>> instead.
>>>>>>
>>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +  aspeed,global-regs:
>>>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>>>>> +    description: The phandle of i2c global register node.
>>>>>>
>>>>>> For what? Same question as usual: do not repeat property name, but
>>>>>> say what is this used for and what exactly it points to.
>>>>>>
>>>>>> s/i2c/I2C/ but then what is "I2C global register node"? This is how
>>>>>> you call your device in datasheet?
>>>>>>
>>>>> I do descript in cover, should add into the yaml file ?
>>>>
>>>>
>>>> Again, cover letter does not matter. Your hardware must be explained here.
>>> Will add into commit.
>>>>
>>>>>
>>>>> aspeed,global-regs:
>>>>> This global register is needed, global register is setting for new
>>>>> clock divide control, and new register set control.
>>>>
>>>> So this means you implement clock controller via syscon?
>>> No, it is just mode switch. It also explain in cover. I will add it in commit.
>>> The legacy register layout is mix controller/target register control together.
>> The following is add more detail description about new register layout. And
>> new feature set add for register.
>>>>
>>>>>
>>>>>>
>>>>>>> +
>>>>>>>  required:
>>>>>>>    - reg
>>>>>>>    - compatible
>>>>>>>    - clocks
>>>>>>>    - resets
>>>>>>>
>>>>>>> +allOf:
>>>>>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>>>>>>> +  - if:
>>>>>>> +      properties:
>>>>>>> +        compatible:
>>>>>>> +          contains:
>>>>>>> +            const: aspeed,ast2600-i2cv2
>>>>>>
>>>>>> NAK, undocumented compatible.
>>>>>
>>>>> Sorry, I should add what kind of document about this compatible?
>>>>
>>>> You cannot add new compatibles without documenting them.
>>>> Documentation is in the form of DT schema and each compatible must be
>>>> listed (in some
>>>> way) in compatible property description.
>>>
>>> Sorry, do you mean, I add following in yaml or commit message?
>>
>> You need to list this in compatibles first.
> 
> I will add it in compatible in next submit.
> 
> 	enum:
> 		-aspeed,ast2600-i2cv2
>>
>>>
>>> This series add AST2600 i2cv2 new register set driver. The i2cv2 driver is new
>> register set that have new clock divider option for more flexiable generation.
>> And also have separate i2c controller and target register set for control, patch
>> #2 is i2c controller driver only, patch #3 is add i2c target mode driver.
>>
>> All this describes driver, not hardware.
> 
> Sorry, the cover letter description the register. I will add int in commit message.
> 
> -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 controller and target mode control.
> -Support 4 individual DMA buffers for controller Tx and Rx,
> target 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 controller DMA command mode.
> -Bus auto timeout for new controller/target 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

I don't understand anything from that and how is this relevant to our
discussion.

> 
>>
>>>
>>> The legacy register layout is mix controller/target 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 controller and target mode control.
>>> -Support 4 individual DMA buffers for controller Tx and Rx, target Tx and Rx.
>>
>> Does it mean hardware changed on AST2600? 
> No Hw change, it is different mode setting will have another mode register setting.
> Mode setting is in global register, I will add in next commit message

Neither this.

So it seems you describe already existing and documented I2C, but for
some reason you want second compatible. The problem is that you do not
provide reason from the point of view of bindings.

To summarize: what your users want - don't care. Start properly
describing hardware and your SoC.


> 
> I2CG00: Device Master Mode Interrupt Status Register (I2CG0C[3]=1)
> I2CG00: Device Master/Slave Mode Interrupt Status Register (I2CG0C[3]=0)
> I2CG04: Device Slave Mode Interrupt Status Register
> I2CG0C: Global Control Register
> I2CG10: New Clock Divider Control Register (I2CG0C[1] = 1)
> 
>> Or these are different devices
>> than aspeed,ast2600-i2c-bus? If this is not a different device, how one SoC can
>> have two different flavors of same device in the same instance?
> 
> When global setting for new, will new register mapping, no setting will keep old register mapping.


I cannot parse this.


Best regards,
Krzysztof

Re: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new register mode driver

[Krzysztof Kozlowski]

On 28/02/2025 13:38, Andy Shevchenko wrote:
> On Fri, Feb 28, 2025 at 09:28:59AM +0800, kernel test robot wrote:
>> Hi Ryan,
>>
>> kernel test robot noticed the following build warnings:
>>
>> [auto build test WARNING on andi-shyti/i2c/i2c-host]
>> [also build test WARNING on linus/master v6.14-rc4 next-20250227]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>
>> url:    https://github.com/intel-lab-lkp/linux/commits/Ryan-Chen/dt-bindings-i2c-aspeed-support-for-AST2600-i2cv2/20250224-140221
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
>> patch link:    https://lore.kernel.org/r/20250224055936.1804279-3-ryan_chen%40aspeedtech.com
>> patch subject: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new register mode driver
>> config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20250228/202502280902.U0gLDhve-lkp@intel.com/config)
>> compiler: mips-linux-gcc (GCC) 14.2.0
>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250228/202502280902.U0gLDhve-lkp@intel.com/reproduce)
>>
>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>> the same patch/commit), kindly add following tags
>> | Reported-by: kernel test robot <lkp@intel.com>
>> | Closes: https://lore.kernel.org/oe-kbuild-all/202502280902.U0gLDhve-lkp@intel.com/
>>
>> All warnings (new ones prefixed by >>):
> 
> My gosh, this is valid report. But it looks like a preexisted issue.
> Can somebody fix this, please?


That was three weeks ago and still no responses from Aspeed or
contributors from here.

I think this tells a lot about aspeedtech.com patchsets on the list.

Best regards,
Krzysztof

Re: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new register mode driver

[Andy Shevchenko]

On Mon, Mar 17, 2025 at 08:48:03AM +0100, Krzysztof Kozlowski wrote:
> On 28/02/2025 13:38, Andy Shevchenko wrote:
> > On Fri, Feb 28, 2025 at 09:28:59AM +0800, kernel test robot wrote:
> >> Hi Ryan,
> >>
> >> kernel test robot noticed the following build warnings:
> >>
> >> [auto build test WARNING on andi-shyti/i2c/i2c-host]
> >> [also build test WARNING on linus/master v6.14-rc4 next-20250227]
> >> [If your patch is applied to the wrong git tree, kindly drop us a note.
> >> And when submitting patch, we suggest to use '--base' as documented in
> >> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >>
> >> url:    https://github.com/intel-lab-lkp/linux/commits/Ryan-Chen/dt-bindings-i2c-aspeed-support-for-AST2600-i2cv2/20250224-140221
> >> base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
> >> patch link:    https://lore.kernel.org/r/20250224055936.1804279-3-ryan_chen%40aspeedtech.com
> >> patch subject: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new register mode driver
> >> config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20250228/202502280902.U0gLDhve-lkp@intel.com/config)
> >> compiler: mips-linux-gcc (GCC) 14.2.0
> >> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250228/202502280902.U0gLDhve-lkp@intel.com/reproduce)
> >>
> >> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> >> the same patch/commit), kindly add following tags
> >> | Reported-by: kernel test robot <lkp@intel.com>
> >> | Closes: https://lore.kernel.org/oe-kbuild-all/202502280902.U0gLDhve-lkp@intel.com/
> >>
> >> All warnings (new ones prefixed by >>):
> > 
> > My gosh, this is valid report. But it looks like a preexisted issue.
> > Can somebody fix this, please?
> 
> 
> That was three weeks ago and still no responses from Aspeed or
> contributors from here.
> 
> I think this tells a lot about aspeedtech.com patchsets on the list.

Yeah...

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new register mode driver

[Ryan Chen]

> Subject: Re: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new register
> mode driver
> 
> On Mon, Mar 17, 2025 at 08:48:03AM +0100, Krzysztof Kozlowski wrote:
> > On 28/02/2025 13:38, Andy Shevchenko wrote:
> > > On Fri, Feb 28, 2025 at 09:28:59AM +0800, kernel test robot wrote:
> > >> Hi Ryan,
> > >>
> > >> kernel test robot noticed the following build warnings:
> > >>
> > >> [auto build test WARNING on andi-shyti/i2c/i2c-host] [also build
> > >> test WARNING on linus/master v6.14-rc4 next-20250227] [If your
> > >> patch is applied to the wrong git tree, kindly drop us a note.
> > >> And when submitting patch, we suggest to use '--base' as documented
> > >> in
> > >> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > >>
> > >> url:
> https://github.com/intel-lab-lkp/linux/commits/Ryan-Chen/dt-bindings-i2c-asp
> eed-support-for-AST2600-i2cv2/20250224-140221
> > >> base:
> https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
> > >> patch link:
> https://lore.kernel.org/r/20250224055936.1804279-3-ryan_chen%40aspeedtec
> h.com
> > >> patch subject: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new
> > >> register mode driver
> > >> config: mips-allyesconfig
> > >> (https://download.01.org/0day-ci/archive/20250228/202502280902.U0gL
> > >> Dhve-lkp@intel.com/config)
> > >> compiler: mips-linux-gcc (GCC) 14.2.0 reproduce (this is a W=1
> > >> build):
> > >> (https://download.01.org/0day-ci/archive/20250228/202502280902.U0gL
> > >> Dhve-lkp@intel.com/reproduce)
> > >>
> > >> If you fix the issue in a separate patch/commit (i.e. not just a
> > >> new version of the same patch/commit), kindly add following tags
> > >> | Reported-by: kernel test robot <lkp@intel.com>
> > >> | Closes:
> > >> | https://lore.kernel.org/oe-kbuild-all/202502280902.U0gLDhve-lkp@i
> > >> | ntel.com/
> > >>
> > >> All warnings (new ones prefixed by >>):
> > >
> > > My gosh, this is valid report. But it looks like a preexisted issue.
> > > Can somebody fix this, please?
> >
> >
> > That was three weeks ago and still no responses from Aspeed or
> > contributors from here.
> >
> > I think this tells a lot about aspeedtech.com patchsets on the list.
> 
Sorry, I don't aware this is my patches issue, could you tell me what build instruction can reproduce this?

> Yeah...
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new register mode driver

[Krzysztof Kozlowski]

On 17/03/2025 09:51, Ryan Chen wrote:
>> Subject: Re: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new register
>> mode driver
>>
>> On Mon, Mar 17, 2025 at 08:48:03AM +0100, Krzysztof Kozlowski wrote:
>>> On 28/02/2025 13:38, Andy Shevchenko wrote:
>>>> On Fri, Feb 28, 2025 at 09:28:59AM +0800, kernel test robot wrote:
>>>>> Hi Ryan,
>>>>>
>>>>> kernel test robot noticed the following build warnings:
>>>>>
>>>>> [auto build test WARNING on andi-shyti/i2c/i2c-host] [also build
>>>>> test WARNING on linus/master v6.14-rc4 next-20250227] [If your
>>>>> patch is applied to the wrong git tree, kindly drop us a note.
>>>>> And when submitting patch, we suggest to use '--base' as documented
>>>>> in
>>>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>>>
>>>>> url:
>> https://github.com/intel-lab-lkp/linux/commits/Ryan-Chen/dt-bindings-i2c-asp
>> eed-support-for-AST2600-i2cv2/20250224-140221
>>>>> base:
>> https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
>>>>> patch link:
>> https://lore.kernel.org/r/20250224055936.1804279-3-ryan_chen%40aspeedtec
>> h.com
>>>>> patch subject: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new
>>>>> register mode driver
>>>>> config: mips-allyesconfig
>>>>> (https://download.01.org/0day-ci/archive/20250228/202502280902.U0gL
>>>>> Dhve-lkp@intel.com/config)
>>>>> compiler: mips-linux-gcc (GCC) 14.2.0 reproduce (this is a W=1
>>>>> build):
>>>>> (https://download.01.org/0day-ci/archive/20250228/202502280902.U0gL
>>>>> Dhve-lkp@intel.com/reproduce)
>>>>>
>>>>> If you fix the issue in a separate patch/commit (i.e. not just a
>>>>> new version of the same patch/commit), kindly add following tags
>>>>> | Reported-by: kernel test robot <lkp@intel.com>
>>>>> | Closes:
>>>>> | https://lore.kernel.org/oe-kbuild-all/202502280902.U0gLDhve-lkp@i
>>>>> | ntel.com/
>>>>>
>>>>> All warnings (new ones prefixed by >>):
>>>>
>>>> My gosh, this is valid report. But it looks like a preexisted issue.
>>>> Can somebody fix this, please?
>>>
>>>
>>> That was three weeks ago and still no responses from Aspeed or
>>> contributors from here.
>>>
>>> I think this tells a lot about aspeedtech.com patchsets on the list.
>>
> Sorry, I don't aware this is my patches issue, could you tell me what build instruction can reproduce this?

Everything is explained in the email. You do not need two people
explaining the report, just read the report.

Best regards,
Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 05/03/2025 10:35, Ryan Chen wrote:
> >> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 27/02/2025 09:19, Ryan Chen wrote:
> >>>>
> >>>>
> >>>>> aspeed,enable-byte:
> >>>>> Force i2c controller use byte mode transfer. the byte mode
> >>>>> transfer will send i2c data each byte by byte, inlcude address
> read/write.
> >>>>
> >>>> Isn't this standard FIFO mode?
> >>> Yes, it is.
> >>>>
> >>>> Why anyone would need to enable byte mode for given board?
> >>> By default, it is buffer-mode, for performance, I don't want user
> >>> enable
> >> byte-mode, it will increase CPU utilize.
> >>> But someone want to be force enable byte-mode, so I add properties.
> >>> https://patchwork.ozlabs.org/project/linux-aspeed/patch/202410070352
> >>> 35 .2254138-3-ryan_chen@aspeedtech.com/
> >>
> >>
> >> I don't see the reason why this would be a board property.
> >>
> >> I understood need for DMA because it is shared and only some of the
> >> controllers can use it. But why choice between buffer and FIFO
> >> depending on hardware?
> >
> > By default, the I2C controller runs in buffer mode. Each i2c bus have 16bytes
> buffer to transmit data.
> > Enabling byte mode will increases CPU utilization, so it is not recommended.
> > However, some user might require forcing byte mode, so I added this
> property to allow that.
> 
> 
> You are not answering the question. I asked why the choice depends on
> hardware and you answer "user might required".
> 
> Do you understand that this is about hardware, not user choices?

The AST2600 I2C controller support 3 modes. 1. Byte mode, 2.buffer mode. 3.dma mode.
Which Buffer vs byte mode. is buffer mode have 16 bytes buffer for each i2c transition.
Byte mode is 1 by 1 transfer.
So, my original submit patch is only for buffer/dma mode selection in property.
But someone in review feedback want to use byte mode, so I add byte mode property now.
If not acceptable, I can keep buffer/dma two mode selection in property.

> 
> 
> >>
> >>
> >>>>
> >>>>>
> >>>>>>
> >>>>>>> +      may require a DTS to manually allocate which controller
> >>>>>>> + can use
> >>>>>> DMA mode.
> >>>>>>> +      The "aspeed,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
> >>>>>>> +      enable DMA mode for that one controller.
> >>>>>>> +
> >>>>>>> +  aspeed,enable-byte:
> >>>>>>> +    type: boolean
> >>>>>>> +    description: |
> >>>>>>> +      I2C bus enable byte mode transfer.
> >>>>>>
> >>>>>> No, either this is expressed as lack of dma mode property or if
> >>>>>> you have three modes, then rather some enum (aspeed,transfer-mode
> >>>>>> ?)
> >>>>>
> >>>>> Thanks suggestion, I will using an enum property like
> >>>>> aspeed,transfer-mode
> >>>> instead.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>> +
> >>>>>>> +  aspeed,global-regs:
> >>>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>>>>> +    description: The phandle of i2c global register node.
> >>>>>>
> >>>>>> For what? Same question as usual: do not repeat property name,
> >>>>>> but say what is this used for and what exactly it points to.
> >>>>>>
> >>>>>> s/i2c/I2C/ but then what is "I2C global register node"? This is
> >>>>>> how you call your device in datasheet?
> >>>>>>
> >>>>> I do descript in cover, should add into the yaml file ?
> >>>>
> >>>>
> >>>> Again, cover letter does not matter. Your hardware must be explained
> here.
> >>> Will add into commit.
> >>>>
> >>>>>
> >>>>> aspeed,global-regs:
> >>>>> This global register is needed, global register is setting for new
> >>>>> clock divide control, and new register set control.
> >>>>
> >>>> So this means you implement clock controller via syscon?
> >>> No, it is just mode switch. It also explain in cover. I will add it in commit.
> >>> The legacy register layout is mix controller/target register control together.
> >> The following is add more detail description about new register
> >> layout. And new feature set add for register.
> >>>>
> >>>>>
> >>>>>>
> >>>>>>> +
> >>>>>>>  required:
> >>>>>>>    - reg
> >>>>>>>    - compatible
> >>>>>>>    - clocks
> >>>>>>>    - resets
> >>>>>>>
> >>>>>>> +allOf:
> >>>>>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> >>>>>>> +  - if:
> >>>>>>> +      properties:
> >>>>>>> +        compatible:
> >>>>>>> +          contains:
> >>>>>>> +            const: aspeed,ast2600-i2cv2
> >>>>>>
> >>>>>> NAK, undocumented compatible.
> >>>>>
> >>>>> Sorry, I should add what kind of document about this compatible?
> >>>>
> >>>> You cannot add new compatibles without documenting them.
> >>>> Documentation is in the form of DT schema and each compatible must
> >>>> be listed (in some
> >>>> way) in compatible property description.
> >>>
> >>> Sorry, do you mean, I add following in yaml or commit message?
> >>
> >> You need to list this in compatibles first.
> >
> > I will add it in compatible in next submit.
> >
> > 	enum:
> > 		-aspeed,ast2600-i2cv2
> >>
> >>>
> >>> This series add AST2600 i2cv2 new register set driver. The i2cv2
> >>> driver is new
> >> register set that have new clock divider option for more flexiable
> generation.
> >> And also have separate i2c controller and target register set for
> >> control, patch
> >> #2 is i2c controller driver only, patch #3 is add i2c target mode driver.
> >>
> >> All this describes driver, not hardware.
> >
> > Sorry, the cover letter description the register. I will add int in commit
> message.
> >
> > -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 controller and target mode control.
> > -Support 4 individual DMA buffers for controller Tx and Rx, target 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 controller DMA command mode.
> > -Bus auto timeout for new controller/target 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
> 
> I don't understand anything from that and how is this relevant to our
> discussion.

Sorry, I don't catch your wanted. 
I assume you want to know new mode register set compare original register set.
> 
> >
> >>
> >>>
> >>> The legacy register layout is mix controller/target 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 controller and target mode control.
> >>> -Support 4 individual DMA buffers for controller Tx and Rx, target Tx and
> Rx.
> >>
> >> Does it mean hardware changed on AST2600?
> > No Hw change, it is different mode setting will have another mode register
> setting.
> > Mode setting is in global register, I will add in next commit message
> 
> Neither this.
> 
> So it seems you describe already existing and documented I2C, but for some
> reason you want second compatible. The problem is that you do not provide
> reason from the point of view of bindings.
> 
> To summarize: what your users want - don't care. Start properly describing
> hardware and your SoC.

OK, for ast2600 i2c controller have two register mode setting.
One, I call it is old register setting, that is right now i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus",
And there have a global register that can set i2c controller as new mode register set. 
That I am going to drive. That I post is all register in new an old register list.

For example, 
Global register [2] = 0 => i2c present as old register set
Global register [2] = 1 => i2c present as new register set
> 
> 
> >
> > I2CG00: Device Master Mode Interrupt Status Register (I2CG0C[3]=1)
> > I2CG00: Device Master/Slave Mode Interrupt Status Register
> > (I2CG0C[3]=0)
> > I2CG04: Device Slave Mode Interrupt Status Register
> > I2CG0C: Global Control Register
> > I2CG10: New Clock Divider Control Register (I2CG0C[1] = 1)
> >
> >> Or these are different devices
> >> than aspeed,ast2600-i2c-bus? If this is not a different device, how
> >> one SoC can have two different flavors of same device in the same instance?
> >
> > When global setting for new, will new register mapping, no setting will keep
> old register mapping.
> 
> 
> I cannot parse this.
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 17/03/2025 10:21, Ryan Chen wrote:
>> Neither this.
>>
>> So it seems you describe already existing and documented I2C, but for some
>> reason you want second compatible. The problem is that you do not provide
>> reason from the point of view of bindings.
>>
>> To summarize: what your users want - don't care. Start properly describing
>> hardware and your SoC.
> 
> OK, for ast2600 i2c controller have two register mode setting.
> One, I call it is old register setting, that is right now i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus",
> And there have a global register that can set i2c controller as new mode register set. 
> That I am going to drive. That I post is all register in new an old register list.
> 
> For example, 
> Global register [2] = 0 => i2c present as old register set
> Global register [2] = 1 => i2c present as new register set
It's the same device though, so the same compatible.

Best regards,
Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 17/03/2025 10:21, Ryan Chen wrote:
> >> Neither this.
> >>
> >> So it seems you describe already existing and documented I2C, but for
> >> some reason you want second compatible. The problem is that you do
> >> not provide reason from the point of view of bindings.
> >>
> >> To summarize: what your users want - don't care. Start properly
> >> describing hardware and your SoC.
> >
> > OK, for ast2600 i2c controller have two register mode setting.
> > One, I call it is old register setting, that is right now i2c-aspeed.c
> > .compatible = "aspeed,ast2600-i2c-bus", And there have a global register
> that can set i2c controller as new mode register set.
> > That I am going to drive. That I post is all register in new an old register list.
> >
> > For example,
> > Global register [2] = 0 => i2c present as old register set Global
> > register [2] = 1 => i2c present as new register set
> It's the same device though, so the same compatible.

Sorry, it is different design, and it share the same register space.
So that the reason add new compatible "aspeed,ast2600-i2cv2" for this driver.
It is different register layout.


> 
> Best regards,
> Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 19/03/2025 12:12, Ryan Chen wrote:
>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>> AST2600-i2cv2
>>
>> On 17/03/2025 10:21, Ryan Chen wrote:
>>>> Neither this.
>>>>
>>>> So it seems you describe already existing and documented I2C, but for
>>>> some reason you want second compatible. The problem is that you do
>>>> not provide reason from the point of view of bindings.
>>>>
>>>> To summarize: what your users want - don't care. Start properly
>>>> describing hardware and your SoC.
>>>
>>> OK, for ast2600 i2c controller have two register mode setting.
>>> One, I call it is old register setting, that is right now i2c-aspeed.c
>>> .compatible = "aspeed,ast2600-i2c-bus", And there have a global register
>> that can set i2c controller as new mode register set.
>>> That I am going to drive. That I post is all register in new an old register list.
>>>
>>> For example,
>>> Global register [2] = 0 => i2c present as old register set Global
>>> register [2] = 1 => i2c present as new register set
>> It's the same device though, so the same compatible.
> 
> Sorry, it is different design, and it share the same register space.
> So that the reason add new compatible "aspeed,ast2600-i2cv2" for this driver.
> It is different register layout.

Which device is described by the existing "aspeed,ast2600-i2c-bus"
compatible? And which device is described by new compatible?



Best regards,
Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 19/03/2025 12:12, Ryan Chen wrote:
> >> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 17/03/2025 10:21, Ryan Chen wrote:
> >>>> Neither this.
> >>>>
> >>>> So it seems you describe already existing and documented I2C, but
> >>>> for some reason you want second compatible. The problem is that you
> >>>> do not provide reason from the point of view of bindings.
> >>>>
> >>>> To summarize: what your users want - don't care. Start properly
> >>>> describing hardware and your SoC.
> >>>
> >>> OK, for ast2600 i2c controller have two register mode setting.
> >>> One, I call it is old register setting, that is right now
> >>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there have
> >>> a global register
> >> that can set i2c controller as new mode register set.
> >>> That I am going to drive. That I post is all register in new an old register
> list.
> >>>
> >>> For example,
> >>> Global register [2] = 0 => i2c present as old register set Global
> >>> register [2] = 1 => i2c present as new register set
> >> It's the same device though, so the same compatible.
> >
> > Sorry, it is different design, and it share the same register space.
> > So that the reason add new compatible "aspeed,ast2600-i2cv2" for this
> driver.
> > It is different register layout.
> 
> Which device is described by the existing "aspeed,ast2600-i2c-bus"
> compatible? And which device is described by new compatible?
> 
On the AST2600 SoC, there are up to 16 I2C controller instances (I2C1 ~ I2C16).
Each of these controllers is hardwired at the SoC level to use either the legacy register layout or the new v2 register layout.
The mode is selected by a bit in the global register, these represent two different hardware blocks:
"aspeed,ast2600-i2c-bus" describes controllers using the legacy register layout.
"aspeed,ast2600-i2cv2" describes controllers using the new register layout
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 24/03/2025 09:30, Ryan Chen wrote:
>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>> AST2600-i2cv2
>>
>> On 19/03/2025 12:12, Ryan Chen wrote:
>>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>>>> AST2600-i2cv2
>>>>
>>>> On 17/03/2025 10:21, Ryan Chen wrote:
>>>>>> Neither this.
>>>>>>
>>>>>> So it seems you describe already existing and documented I2C, but
>>>>>> for some reason you want second compatible. The problem is that you
>>>>>> do not provide reason from the point of view of bindings.
>>>>>>
>>>>>> To summarize: what your users want - don't care. Start properly
>>>>>> describing hardware and your SoC.
>>>>>
>>>>> OK, for ast2600 i2c controller have two register mode setting.
>>>>> One, I call it is old register setting, that is right now
>>>>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there have
>>>>> a global register
>>>> that can set i2c controller as new mode register set.
>>>>> That I am going to drive. That I post is all register in new an old register
>> list.
>>>>>
>>>>> For example,
>>>>> Global register [2] = 0 => i2c present as old register set Global
>>>>> register [2] = 1 => i2c present as new register set
>>>> It's the same device though, so the same compatible.
>>>
>>> Sorry, it is different design, and it share the same register space.
>>> So that the reason add new compatible "aspeed,ast2600-i2cv2" for this
>> driver.
>>> It is different register layout.
>>
>> Which device is described by the existing "aspeed,ast2600-i2c-bus"
>> compatible? And which device is described by new compatible?
>>
> On the AST2600 SoC, there are up to 16 I2C controller instances (I2C1 ~ I2C16).

So you have 16 same devices.

> Each of these controllers is hardwired at the SoC level to use either the legacy register layout or the new v2 register layout.
> The mode is selected by a bit in the global register, these represent two different hardware blocks:
> "aspeed,ast2600-i2c-bus" describes controllers using the legacy register layout.
> "aspeed,ast2600-i2cv2" describes controllers using the new register layout

Which part of "same device" is not clear? You have one device, one
compatible. Whatever you do with register layout, is already defined by
that compatible. It does not matter that you forgot to implement it in
the Linux kernel.

Best regards,
Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 24/03/2025 09:30, Ryan Chen wrote:
> >> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 19/03/2025 12:12, Ryan Chen wrote:
> >>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >>>> AST2600-i2cv2
> >>>>
> >>>> On 17/03/2025 10:21, Ryan Chen wrote:
> >>>>>> Neither this.
> >>>>>>
> >>>>>> So it seems you describe already existing and documented I2C, but
> >>>>>> for some reason you want second compatible. The problem is that
> >>>>>> you do not provide reason from the point of view of bindings.
> >>>>>>
> >>>>>> To summarize: what your users want - don't care. Start properly
> >>>>>> describing hardware and your SoC.
> >>>>>
> >>>>> OK, for ast2600 i2c controller have two register mode setting.
> >>>>> One, I call it is old register setting, that is right now
> >>>>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there
> >>>>> have a global register
> >>>> that can set i2c controller as new mode register set.
> >>>>> That I am going to drive. That I post is all register in new an
> >>>>> old register
> >> list.
> >>>>>
> >>>>> For example,
> >>>>> Global register [2] = 0 => i2c present as old register set Global
> >>>>> register [2] = 1 => i2c present as new register set
> >>>> It's the same device though, so the same compatible.
> >>>
> >>> Sorry, it is different design, and it share the same register space.
> >>> So that the reason add new compatible "aspeed,ast2600-i2cv2" for
> >>> this
> >> driver.
> >>> It is different register layout.
> >>
> >> Which device is described by the existing "aspeed,ast2600-i2c-bus"
> >> compatible? And which device is described by new compatible?
> >>
> > On the AST2600 SoC, there are up to 16 I2C controller instances (I2C1 ~
> I2C16).
> 
> So you have 16 same devices.
Yes, one driver instance for 16 i2c device. 
> 
> > Each of these controllers is hardwired at the SoC level to use either the
> legacy register layout or the new v2 register layout.
> > The mode is selected by a bit in the global register, these represent two
> different hardware blocks:
> > "aspeed,ast2600-i2c-bus" describes controllers using the legacy register
> layout.
> > "aspeed,ast2600-i2cv2" describes controllers using the new register
> > layout
> 
> Which part of "same device" is not clear? You have one device, one compatible.
> Whatever you do with register layout, is already defined by that compatible. It
> does not matter that you forgot to implement it in the Linux kernel.

Sorry, I still can't catch your point.
I separated the support into two drivers:
i2c-aspeed.c for legacy layout, compatible "aspeed,ast2600-i2c-bus"
i2c-ast2600.c for the new register set, compatible compatible "aspeed,ast2600-i2cv2"
Do you mean I have integrate into 1 driver at i2c-aspeed.c ? can't be separate two driver?
> 
> Best regards,
> Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 24/03/2025 11:01, Ryan Chen wrote:
>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>> AST2600-i2cv2
>>
>> On 24/03/2025 09:30, Ryan Chen wrote:
>>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>>>> AST2600-i2cv2
>>>>
>>>> On 19/03/2025 12:12, Ryan Chen wrote:
>>>>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>>>>>> AST2600-i2cv2
>>>>>>
>>>>>> On 17/03/2025 10:21, Ryan Chen wrote:
>>>>>>>> Neither this.
>>>>>>>>
>>>>>>>> So it seems you describe already existing and documented I2C, but
>>>>>>>> for some reason you want second compatible. The problem is that
>>>>>>>> you do not provide reason from the point of view of bindings.
>>>>>>>>
>>>>>>>> To summarize: what your users want - don't care. Start properly
>>>>>>>> describing hardware and your SoC.
>>>>>>>
>>>>>>> OK, for ast2600 i2c controller have two register mode setting.
>>>>>>> One, I call it is old register setting, that is right now
>>>>>>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there
>>>>>>> have a global register
>>>>>> that can set i2c controller as new mode register set.
>>>>>>> That I am going to drive. That I post is all register in new an
>>>>>>> old register
>>>> list.
>>>>>>>
>>>>>>> For example,
>>>>>>> Global register [2] = 0 => i2c present as old register set Global
>>>>>>> register [2] = 1 => i2c present as new register set
>>>>>> It's the same device though, so the same compatible.
>>>>>
>>>>> Sorry, it is different design, and it share the same register space.
>>>>> So that the reason add new compatible "aspeed,ast2600-i2cv2" for
>>>>> this
>>>> driver.
>>>>> It is different register layout.
>>>>
>>>> Which device is described by the existing "aspeed,ast2600-i2c-bus"
>>>> compatible? And which device is described by new compatible?
>>>>
>>> On the AST2600 SoC, there are up to 16 I2C controller instances (I2C1 ~
>> I2C16).
>>
>> So you have 16 same devices.
> Yes, one driver instance for 16 i2c device. 
>>
>>> Each of these controllers is hardwired at the SoC level to use either the
>> legacy register layout or the new v2 register layout.
>>> The mode is selected by a bit in the global register, these represent two
>> different hardware blocks:
>>> "aspeed,ast2600-i2c-bus" describes controllers using the legacy register
>> layout.
>>> "aspeed,ast2600-i2cv2" describes controllers using the new register
>>> layout
>>
>> Which part of "same device" is not clear? You have one device, one compatible.
>> Whatever you do with register layout, is already defined by that compatible. It
>> does not matter that you forgot to implement it in the Linux kernel.
> 
> Sorry, I still can't catch your point.


I repeated twice "You have one device, one compatible.", provided few
more sentences and if this is still unclear, I don't know what to say more.

> I separated the support into two drivers:

I don't care about your drivers, why are you bringing them here?

> i2c-aspeed.c for legacy layout, compatible "aspeed,ast2600-i2c-bus"
> i2c-ast2600.c for the new register set, compatible compatible "aspeed,ast2600-i2cv2"
> Do you mean I have integrate into 1 driver at i2c-aspeed.c ? can't be separate two driver?

What is this patch about? Bindings. Not drivers. Look at email month ago:

"All this describes driver, not hardware."

Why are you keep bringing here drivers since a month?

Best regards,
Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 24/03/2025 11:01, Ryan Chen wrote:
> >> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 24/03/2025 09:30, Ryan Chen wrote:
> >>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >>>> AST2600-i2cv2
> >>>>
> >>>> On 19/03/2025 12:12, Ryan Chen wrote:
> >>>>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support
> >>>>>> for
> >>>>>> AST2600-i2cv2
> >>>>>>
> >>>>>> On 17/03/2025 10:21, Ryan Chen wrote:
> >>>>>>>> Neither this.
> >>>>>>>>
> >>>>>>>> So it seems you describe already existing and documented I2C,
> >>>>>>>> but for some reason you want second compatible. The problem is
> >>>>>>>> that you do not provide reason from the point of view of bindings.
> >>>>>>>>
> >>>>>>>> To summarize: what your users want - don't care. Start properly
> >>>>>>>> describing hardware and your SoC.
> >>>>>>>
> >>>>>>> OK, for ast2600 i2c controller have two register mode setting.
> >>>>>>> One, I call it is old register setting, that is right now
> >>>>>>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there
> >>>>>>> have a global register
> >>>>>> that can set i2c controller as new mode register set.
> >>>>>>> That I am going to drive. That I post is all register in new an
> >>>>>>> old register
> >>>> list.
> >>>>>>>
> >>>>>>> For example,
> >>>>>>> Global register [2] = 0 => i2c present as old register set
> >>>>>>> Global register [2] = 1 => i2c present as new register set
> >>>>>> It's the same device though, so the same compatible.
> >>>>>
> >>>>> Sorry, it is different design, and it share the same register space.
> >>>>> So that the reason add new compatible "aspeed,ast2600-i2cv2" for
> >>>>> this
> >>>> driver.
> >>>>> It is different register layout.
> >>>>
> >>>> Which device is described by the existing "aspeed,ast2600-i2c-bus"
> >>>> compatible? And which device is described by new compatible?
> >>>>
> >>> On the AST2600 SoC, there are up to 16 I2C controller instances
> >>> (I2C1 ~
> >> I2C16).
> >>
> >> So you have 16 same devices.
> > Yes, one driver instance for 16 i2c device.
> >>
> >>> Each of these controllers is hardwired at the SoC level to use
> >>> either the
> >> legacy register layout or the new v2 register layout.
> >>> The mode is selected by a bit in the global register, these
> >>> represent two
> >> different hardware blocks:
> >>> "aspeed,ast2600-i2c-bus" describes controllers using the legacy
> >>> register
> >> layout.
> >>> "aspeed,ast2600-i2cv2" describes controllers using the new register
> >>> layout
> >>
> >> Which part of "same device" is not clear? You have one device, one
> compatible.
> >> Whatever you do with register layout, is already defined by that
> >> compatible. It does not matter that you forgot to implement it in the Linux
> kernel.
> >
> > Sorry, I still can't catch your point.
> 
> 
> I repeated twice "You have one device, one compatible.", provided few more
> sentences and if this is still unclear, I don't know what to say more.
> 
> > I separated the support into two drivers:
> 
> I don't care about your drivers, why are you bringing them here?
> 
> > i2c-aspeed.c for legacy layout, compatible "aspeed,ast2600-i2c-bus"
> > i2c-ast2600.c for the new register set, compatible compatible
> "aspeed,ast2600-i2cv2"
> > Do you mean I have integrate into 1 driver at i2c-aspeed.c ? can't be
> separate two driver?
> 
> What is this patch about? Bindings. Not drivers. Look at email month ago:
> 
> "All this describes driver, not hardware."
> 
> Why are you keep bringing here drivers since a month?
Let me try to rephrase the reasoning from a hardware point of view:

On AST2600, each I2C controller instance is functionally the same type of device (I2C controller), 
but it can present two different and incompatible register layouts, 
depending on a hardware-controlled configuration (via global register bits that are fixed in production SoCs).
The layouts are not backward-compatible:
Registers are at different offsets. Bit definitions differ.
The programming sequence is not the same.
Because of this incompatibility at the register level, software cannot handle both layouts in a generic way without explicit knowledge of which version is present.
That’s why I proposed a new compatible string — to represent a hardware-visible difference in register interface, even though the logical function (I2C controller) is the same.


> 
> Best regards,
> Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 25/03/2025 10:52, Ryan Chen wrote:
>>
>>> i2c-aspeed.c for legacy layout, compatible "aspeed,ast2600-i2c-bus"
>>> i2c-ast2600.c for the new register set, compatible compatible
>> "aspeed,ast2600-i2cv2"
>>> Do you mean I have integrate into 1 driver at i2c-aspeed.c ? can't be
>> separate two driver?
>>
>> What is this patch about? Bindings. Not drivers. Look at email month ago:
>>
>> "All this describes driver, not hardware."
>>
>> Why are you keep bringing here drivers since a month?
> Let me try to rephrase the reasoning from a hardware point of view:
> 
> On AST2600, each I2C controller instance is functionally the same type of device (I2C controller), 
> but it can present two different and incompatible register layouts, 
> depending on a hardware-controlled configuration (via global register bits that are fixed in production SoCs).
> The layouts are not backward-compatible:
> Registers are at different offsets. Bit definitions differ.
> The programming sequence is not the same.
> Because of this incompatibility at the register level, software cannot handle both layouts in a generic way without explicit knowledge of which version is present.
> That’s why I proposed a new compatible string — to represent a hardware-visible difference in register interface, even though the logical function (I2C controller) is the same.

You implied software is set in stone and that's not true. You have a
compatible which says to the software - this device has two programming
interfaces, choose whatever you wish and work with that.

Different compatible would mean the device is different, it changes or
you have compatibility issues.

NONE of these cases are here.

I am done with this topic, because explaining this and other
trivialities to Aspeed takes way too much time, so last opinion:

Your compatible already expressed that there are two interfaces, so your
drivers can just choose whichever they want. If you need to toggle a bit
in system controller, it is fine. If you need different compatible, then
that's a NAK.

Best regards,
Krzysztof