drivers: i2c: rts5912 i2c dirver #89386

Titan-Realtek · 2025-05-02T08:14:07Z

base on DesignWare I2C driver to implement RTS5912 I2C driver.

support customize bus recovery function.
fix isr timing issue by enable tx empty control.
support stuck at low handle by enable bus clear feature.
support custom stuck at low timeout set from dts
disable block mode in rts5912 i2c.
support I2C_ALLOW_NO_STOP_TRANSACTIONS

keith-zephyr · 2025-05-02T18:02:55Z

drivers/i2c/Kconfig

@@ -61,7 +61,7 @@ config I2C_ALLOW_NO_STOP_TRANSACTIONS
 	depends on !I2C_STM32
 	depends on !I2C_GD32
 	depends on !I2C_ESP32
-	depends on !I2C_DW
+	depends on (!I2C_DW || I2C_RTS5912)


CONFIG_I2C_ALLOW_NO_STOP_TRANSACTIONS is deprecated and will be removed. The RTS5912 driver needs to work without this option.

Hi, Keith
currently I need to enable the CONFIG to allow driver do not send a STOP bit at a special i2c msg combination,
such as CROS i2c_controller.c , i2c_read_sized_block() funciton, the first message transaction only has START flag and without STOP flag :

/* * Send device reg space offset, and read back block length. * Keep this session open without a stop. */ rv = i2c_xfer_unlocked(port, addr_flags, &reg, 1, &block_length, 1, I2C_XFER_START);

if I don't enable this CONFIG , ZEPHYR i2c transport layer will force to add STOP bit

if (!IS_ENABLED(CONFIG_I2C_ALLOW_NO_STOP_TRANSACTIONS)) { msgs[num_msgs - 1].flags |= I2C_MSG_STOP; }

then after first transaction done, the dw i2c ip will setup again and send a START+ADDR at second message.
so, I need to enable the flag and combine the new variable "need_setup" to control the IP don't setup again and continue to read the data with STOP flag.

The plan to deprecate CONFIG_I2C_ALLOW_NO_STOP_TRANSACTIONS has been delayed. This is okay for now.

I got it, thanks.

drivers/i2c/Kconfig.rts5912

drivers/i2c/i2c_realtek_rts5912.c

drivers/i2c/i2c_dw_registers.h

keith-zephyr · 2025-05-02T19:27:16Z

drivers/i2c/i2c_dw.h

 #define DW_ENABLE_TX_INT_I2C_MASTER                                                                \
 	(DW_INTR_STAT_TX_OVER | DW_INTR_STAT_TX_EMPTY | DW_INTR_STAT_TX_ABRT |                     \
-	 DW_INTR_STAT_STOP_DET)
+	 DW_INTR_STAT_STOP_DET | DW_INTR_STAT_SCL_STUCK_LOW)


Suggested change

#define DW_ENABLE_TX_INT_I2C_MASTER \

(DW_INTR_STAT_TX_OVER | DW_INTR_STAT_TX_EMPTY | DW_INTR_STAT_TX_ABRT | \

DW_INTR_STAT_STOP_DET)

DW_INTR_STAT_STOP_DET | DW_INTR_STAT_SCL_STUCK_LOW)

#ifdef CONFIG_I2C_DW_EXTENDED_SUPPORT

#define DW_ENABLE_TX_INT_I2C_MASTER \

(DW_INTR_STAT_TX_OVER | DW_INTR_STAT_TX_EMPTY | DW_INTR_STAT_TX_ABRT | \

DW_INTR_STAT_STOP_DET | DW_INTR_STAT_SCL_STUCK_LOW)

#else

#define DW_ENABLE_TX_INT_I2C_MASTER \

(DW_INTR_STAT_TX_OVER | DW_INTR_STAT_TX_EMPTY | DW_INTR_STAT_TX_ABRT | \

DW_INTR_STAT_STOP_DET)

#endif

Copilot

Pull Request Overview

This PR implements the Realtek RTS5912 I2C driver based on the DesignWare I2C driver and enhances bus recovery, error handling, and initialization. Key changes include support for a customizable bus recovery function (including stuck‐at‐low handling and bus clear), a fix for ISR timing issues via TX empty control, and the integration of a new DTS binding with additional timeout and GPIO parameters.

Reviewed Changes

Copilot reviewed 6 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
dts/bindings/i2c/realtek,rts5912-i2c.yaml	Adds DTS binding for RTS5912 with new properties for timeout values and GPIO pins.
drivers/i2c/i2c_realtek_rts5912.h	Introduces the header file for the RTS5912 driver.
drivers/i2c/i2c_realtek_rts5912.c	Implements the RTS5912 driver functions including bus recovery and initialization.
drivers/i2c/i2c_dw_registers.h	Updates register definitions and adds new bit-fields used by the RTS5912 driver.
drivers/i2c/i2c_dw.h	Adjusts the DW driver header structures and replaces mutex usage with a semaphore.
drivers/i2c/i2c_dw.c	Updates the DW core implementation with enhanced error handling, recovery callbacks, and a fix for pending interrupt clearing.

Files not reviewed (5)

drivers/i2c/CMakeLists.txt: Language not supported
drivers/i2c/Kconfig: Language not supported
drivers/i2c/Kconfig.dw: Language not supported
drivers/i2c/Kconfig.rts5912: Language not supported
dts/arm/realtek/ec/rts5912.dtsi: Language not supported

Copilot · 2025-05-03T01:01:15Z

drivers/i2c/i2c_realtek_rts5912.c

+			LOG_ERR("ERROR: ABORT Fail!");
+			ret = false;
+		} else {
+			LOG_DBG("ABORT seccuess");


Typo in log message: 'seccuess' should be changed to 'success'.

Suggested change

LOG_DBG("ABORT seccuess");

LOG_DBG("ABORT success");

Copilot · 2025-05-03T01:01:15Z

drivers/i2c/i2c_realtek_rts5912.c

+		LOG_ERR("ERROR: ABORT Fail!");
+		ret = false;
+	} else {
+		LOG_DBG("ABORT seccuess");


Typo in log message: 'seccuess' should be changed to 'success'.

Suggested change

LOG_DBG("ABORT seccuess");

LOG_DBG("ABORT success");

Copilot · 2025-05-03T01:01:15Z

drivers/i2c/i2c_dw.c

 	uint32_t value;
 	uint32_t reg_base = get_regs(dev);

 	write_intr_mask(DW_DISABLE_ALL_I2C_INT, reg_base);
 	value = read_clr_intr(reg_base);
-
+	/* clear pedning interrupt */


Typo in the comment: 'pedning' should be corrected to 'pending'.

Suggested change

/* clear pedning interrupt */

/* clear pending interrupt */

Copilot · 2025-05-03T01:01:16Z

drivers/i2c/i2c_realtek_rts5912.c

+
+	uint32_t value;
+	uint32_t start;
+	int ret = true;


[nitpick] Consider initializing 'ret' with 0 for success rather than 'true' to clearly indicate that 0 is the success code.

Suggested change

int ret = true;

int ret = 0;

keith-zephyr · 2025-05-13T19:34:23Z

drivers/i2c/Kconfig.rts5912

+# Copyright (c) 2025 Realtek, SIBG-SD7
+# SPDX-License-Identifier: Apache-2.0
+
+config I2C_DW_EXTENDED_SUPPORT


This should be defined in Kconfig.dw

keith-zephyr · 2025-05-13T19:56:03Z

drivers/i2c/i2c_realtek_rts5912.c

+	uint32_t sda_timeout = config->sda_timeout_value * CONFIG_I2C_DW_CLOCK_SPEED * 1000;
+	uint32_t scl_timeout = config->scl_timeout_value * CONFIG_I2C_DW_CLOCK_SPEED * 1000;


This can be moved into the i2c.dw.c, guarded with CONFIG_I2C_DW_EXTENDED_SUPPORT

keith-zephyr · 2025-05-13T19:56:56Z

drivers/i2c/i2c_realtek_rts5912.c

+#define REALTEK_RTS5912_PINMUX_GET_GPIO_PIN(n)                                                     \
+	(((((n) >> REALTEK_RTS5912_GPIO_LOW_POS) & REALTEK_RTS5912_GPIO_LOW_MSK)) |                \
+	 (((((n) >> REALTEK_RTS5912_GPIO_HIGH_POS) & REALTEK_RTS5912_GPIO_HIGH_MSK)) << 5))


This isn't used.

keith-zephyr · 2025-05-13T20:12:14Z

drivers/i2c/i2c_realtek_rts5912.c

+	/* set high level to scl and sda line */
+	*GPIO_SCL = 0x28003;
+	*GPIO_SDA = 0x28003;


The i2c_bitbang driver already provides the code to recovery the bus using GPIOs.

You don't need to write into the Realtek GPIO registers directly.

Look at the existing uses of i2c_bitbang_recover_bus() in the tree for examples of you to use the driver.

I tried using bitbang to recover the bus signal but the RTMR clock is too low to calculate the delayed HW cycles in i2c_bitbang.c.
Therefore, I still used my own code to restore the bus signal.

You still shouldn't access the GPIO registers directly. The devicetree node should include phandles for the GPIO pins used, and then you can use gpio_pin_get_dt to read the pin state and gpio_pin_set_dt to change the pin state.

You will also need to use gpio_pin_configure at the start of the recover sequence and then call pinctrl_apply_state at the end to restore the pins back to the I2C function.

The ITE I2C driver provides an example of this

drivers/i2c/i2c_dw.c

keith-zephyr · 2025-05-16T17:25:14Z

drivers/i2c/Kconfig

@@ -61,7 +61,7 @@ config I2C_ALLOW_NO_STOP_TRANSACTIONS
 	depends on !I2C_STM32
 	depends on !I2C_GD32
 	depends on !I2C_ESP32
-	depends on !I2C_DW
+	depends on (!I2C_DW || I2C_RTS5912)


The plan to deprecate CONFIG_I2C_ALLOW_NO_STOP_TRANSACTIONS has been delayed. This is okay for now.

keith-zephyr · 2025-05-16T17:28:18Z

dts/bindings/i2c/realtek,rts5912-i2c.yaml

+    description: |
+      DesignWare i2c device
+
+  sda-gpio-pin:


This should use the a GPIO phandles;

scl-gpios: type: phandle-array required: true description: | The SCL pin for the selected port. sda-gpios: type: phandle-array required: true description: | The SDA pin for the selected port.

its kind of horrible to be frank that our devicetree setup wants this to be "gpios" plural (we regex on this suffix...) with a phandle-array type, but yes this is the right way to do it for zephyr

keith-zephyr · 2025-05-16T17:34:43Z

drivers/i2c/i2c_realtek_rts5912.c

+	/* set high level to scl and sda line */
+	*GPIO_SCL = 0x28003;
+	*GPIO_SDA = 0x28003;


You still shouldn't access the GPIO registers directly. The devicetree node should include phandles for the GPIO pins used, and then you can use gpio_pin_get_dt to read the pin state and gpio_pin_set_dt to change the pin state.

You will also need to use gpio_pin_configure at the start of the recover sequence and then call pinctrl_apply_state at the end to restore the pins back to the I2C function.

The ITE I2C driver provides an example of this

keith-zephyr · 2025-05-16T17:37:06Z

drivers/i2c/i2c_dw.c

-
+#ifdef CONFIG_CPU_CORTEX_M
+	/* clear pending interrupt */
+	NVIC_ClearPendingIRQ(rom->irqnumber);


@teburd Is there an architecture generic call to clear a pending interrupt?

I don't see anything in zephyr/arch/arch_interface.h for this, and I see a lot of drivers directly manipulating the nvic with these cmsis functions, so I'd say no

yes, I also see a lot of drivers directly call the nvic funcitons, but I got [Run tests with twister / twister-build (1) (pull_request)] Failed if I don't use CONFIG_CPU_CORTEX_M to separate.
does anyone know what's happen?

This driver is compiled by multiple architectures. NVIC_ClearPendingIRQ() is only provided by the CORTEX_M architecture.

I try CONFIG_ARM to test but [Run tests with twister / twister-build (1) (pull_request)] still failed, so I back to use CONFIG_CPU_CORTEX_M

keith-zephyr · 2025-05-16T17:45:12Z

drivers/i2c/i2c_dw.c

-	if (ret) {
-		goto error;
+#if CONFIG_I2C_ALLOW_NO_STOP_TRANSACTIONS
+	if (dw->need_setup) {


Move this check into i2c_dw_setup. It will make the code easier to read.

static int i2c_dw_setup(const struct device *dev, uint16_t slave_address) { #if CONFIG_I2C_ALLOW_NO_STOP_TRANSACTIONS if (!dw->need_setup) { return 0; } #endif /* Existing code ... */ #if CONFIG_I2C_ALLOW_NO_STOP_TRANSACTIONS dw->need_setup = false #endif return 0; }

keith-zephyr · 2025-05-16T17:53:59Z

drivers/i2c/i2c_dw.c

+	if ((test_bit_status_activity(reg_base) || (dw->state & I2C_DW_BUSY))
+#if CONFIG_I2C_ALLOW_NO_STOP_TRANSACTIONS
+	    && dw->need_setup
+#endif
+	) {


The inline #ifdef here is hard to read.

Create a helper function to hide the details.

bool i2c_dw_is_busy(dev) { struct i2c_dw_dev_config *const dw = dev->data; #if CONFIG_I2C_ALLOW_NO_STOP_TRANSACTIONS /* The application explicitly started a transaction without * a STOP. Allow the application to continue sending transactions. */ if (!dw->need_setup) { return false; } #endif if ((test_bit_status_activity(reg_base) || (dw->state & I2C_DW_BUSY)) { return true; } return false; } 111

keith-zephyr · 2025-05-16T17:55:51Z

drivers/i2c/i2c_dw.c

+		.sda_timeout_value = DT_INST_PROP_OR(n, sda_timeout_value, 0),                     \
+		.scl_timeout_value = DT_INST_PROP_OR(n, scl_timeout_value, 0),                     \


Guard with CONFIG_I2C_DW_EXTENDED_SUPPORT

drivers/i2c/i2c_dw.h

keith-zephyr

This is big improvement over the original copy/pasted driver. Thank you for making the changes.

Overall, looks pretty good. I do wonder whether the i2c_bitbang support for bus recovery can be enhanced to work with a slower system clock instead of creating a realtek specific implementation.

keith-zephyr · 2025-05-21T00:04:23Z

drivers/i2c/i2c_realtek_rts5912.c

+	/* 9 cycles of SCL with SDA held high */
+	for (i = 0; i < 9; i++) {
+		gpio_pin_set_dt(&config->scl_gpios, 1);
+		k_busy_wait(50);


Do these delays need to be different based on the configured I2C clock rate?

Hi, Keith
Since we don't know what the external pull-up resistor is, I set the clock to a low enough frequency, like 10KHz, to ensure the waveform has enough rise time for all devices total capacitance.

keith-zephyr · 2025-05-28T20:20:02Z

drivers/i2c/i2c_dw.c

@@ -341,12 +398,18 @@ static int i2c_dw_data_send(const struct device *dev)
 static inline void i2c_dw_transfer_complete(const struct device *dev)
 {
 	struct i2c_dw_dev_config *const dw = dev->data;
+#ifdef CONFIG_CPU_CORTEX_M
+	const struct i2c_dw_rom_config *const rom = dev->config;


Nit - move the rom definition into the #ifdef block at line 409.

keith-zephyr · 2025-05-28T20:28:48Z

drivers/i2c/i2c_dw.c

+			} else if (dw->state & (I2C_DW_NACK | I2C_DW_CMD_ERROR)) {
+				ret = -EIO;
+			}
+			k_sem_reset(&dw->device_sync_sem);


Other I2C transfer errors are not resetting the semaphore. I don't think this is needed on this path.

base on DesignWare I2C driver to implement RTS5912 I2C driver. 1. support customize bus recovery function. 2. fix isr timing issue by enable tx empty control. 3. support stuck at low handle by enable bus clear feature. 4. support custom stuck at low timeout set from dts 5. disable block mode in rts5912 i2c. 6. support I2C_ALLOW_NO_STOP_TRANSACTIONS Signed-off-by: Titan Chen <[email protected]>

support errors check for 1. tx_abrt: nack and sda stuck low 2. scl stuck low Signed-off-by: Titan Chen <[email protected]>

sonarqubecloud · 2025-05-29T06:31:37Z

Quality Gate passed

Issues
16 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-actions bot added area: I2C platform: Realtek EC labels May 2, 2025

github-actions bot requested review from JasonLin-RealTek and teburd May 2, 2025 08:14

github-actions bot assigned teburd May 2, 2025

keith-zephyr reviewed May 2, 2025

View reviewed changes

kartben requested a review from Copilot May 3, 2025 01:00

Copilot AI reviewed May 3, 2025

View reviewed changes

Titan-Realtek force-pushed the drivers_i2c_dw_rts5912 branch 12 times, most recently from f82d638 to 3851d51 Compare May 8, 2025 00:06

keith-zephyr requested changes May 13, 2025

View reviewed changes

Titan-Realtek force-pushed the drivers_i2c_dw_rts5912 branch 4 times, most recently from 1d198d2 to afe4808 Compare May 14, 2025 12:42

keith-zephyr reviewed May 16, 2025

View reviewed changes

Titan-Realtek force-pushed the drivers_i2c_dw_rts5912 branch 4 times, most recently from e19183c to 3e37c3a Compare May 19, 2025 05:38

keith-zephyr reviewed May 21, 2025

View reviewed changes

keith-zephyr mentioned this pull request May 21, 2025

drivers: i2c: add supprt rts5912 i2c #87106

Closed

Titan-Realtek force-pushed the drivers_i2c_dw_rts5912 branch 2 times, most recently from 5212ed2 to 582d0c8 Compare May 22, 2025 06:16

keith-zephyr reviewed May 28, 2025

View reviewed changes

Titan-Realtek added 2 commits May 29, 2025 10:39

drivers: i2c: add i2c dw support error checks.

ddf91b8

support errors check for 1. tx_abrt: nack and sda stuck low 2. scl stuck low Signed-off-by: Titan Chen <[email protected]>

Titan-Realtek force-pushed the drivers_i2c_dw_rts5912 branch from 582d0c8 to ddf91b8 Compare May 29, 2025 05:36

keith-zephyr approved these changes May 29, 2025

View reviewed changes

teburd approved these changes May 29, 2025

View reviewed changes

kartben merged commit 13a0242 into zephyrproject-rtos:main May 29, 2025
26 checks passed

		uint32_t sda_timeout = config->sda_timeout_value * CONFIG_I2C_DW_CLOCK_SPEED * 1000;
		uint32_t scl_timeout = config->scl_timeout_value * CONFIG_I2C_DW_CLOCK_SPEED * 1000;

		.sda_timeout_value = DT_INST_PROP_OR(n, sda_timeout_value, 0), \
		.scl_timeout_value = DT_INST_PROP_OR(n, scl_timeout_value, 0), \

drivers: i2c: rts5912 i2c dirver #89386

drivers: i2c: rts5912 i2c dirver #89386

Uh oh!

Conversation

Titan-Realtek commented May 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Titan-Realtek May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Titan-Realtek May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Titan-Realtek May 5, 2025 •

edited

Loading

Titan-Realtek May 14, 2025 •

edited

Loading