soc : realtek: ec: rts5912: add support ULPM #86717

Titan-Realtek · 2025-03-06T11:25:12Z

Port rts5912 ULPM on Zephyr

nordicjm · 2025-03-07T08:53:16Z

dts/bindings/power/realtek,rts5912-ulpm.yaml

+  status:
+    type: string
+    description: indicates the operational status of a device
+    enum:
+      - "ok" # Deprecated form
+      - "okay"
+      - "disabled"
+      - "reserved"
+      - "fail"
+      - "fail-sss"
+
+  compatible:
+    type: string-array
+    required: true
+    description: compatible strings


Do not do this, include the base definition properly

done, thanks.

nordicjm · 2025-03-07T08:53:52Z

soc/realtek/ec/rts5912/Kconfig

@@ -9,6 +9,7 @@ config SOC_SERIES_RTS5912
 	select SYS_CLOCK_EXISTS
 	select DYNAMIC_INTERRUPTS
 	select SOC_EARLY_INIT_HOOK
+	select HAS_POWEROFF if(SOC_RTS5912_ULPM)


Suggested change

select HAS_POWEROFF if(SOC_RTS5912_ULPM)

done, thanks.

nordicjm · 2025-03-07T08:55:18Z

dts/arm/realtek/ec/rts5912.dtsi

+
+		wkup-pin@0 {
+			reg = <0x0>;
+			/* wkup-pin-pol; */


commented lines to go?

done, thanks.

nordicjm · 2025-03-07T08:55:54Z

soc/realtek/ec/rts5912/Kconfig

+	bool "Realtek RTS5912 ULPM (Ultra Low Power Mode)"
+	default y
+	depends on DT_HAS_REALTEK_RTS5912_ULPM_ENABLED
+	select POWEROFF


Suggested change

select POWEROFF

select HAS_POWEROFF

Users select POWEROFF if they want it not a soc

done, thanks.

keith-zephyr · 2025-03-27T15:16:39Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+
+#define RTS5912_ULPM_NODE DT_NODELABEL(ulpm)
+
+#define ULPM_RTS5912_MAX_NB_WKUP_PINS DT_INST_PROP(0, wkup_pins_nb)


You can replace the wkup-pins-nb property with the macro DT_INST_CHILD_NUM_STATUS_OKAY(0).

Hi Keith
The wkup-pins-nb is used to check whether the id assigned to a child node exceeds the boundary.
There is another definition to calculate the child number.

zephyr/soc/realtek/ec/rts5912/rts5912_ulpm.c

Line 87 in 7d15a9a

#define WKUP_PIN_SIZE ARRAY_SIZE(wkup_pins_cfgs)

The max wkup pin number can prevent if someone add another new child and assign a wrong id number.

I suggest renaming the property wkup-pins-max.

keith-zephyr · 2025-03-27T15:21:29Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+
+#define ULPM_SLEEP(x) k_msleep(x)
+
+#define RTS5912_ULPM_NODE DT_NODELABEL(ulpm)


Use DT_DRV_INST(0) instead of the nodelabel.

keith-zephyr · 2025-03-27T15:23:25Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+/** @endcond */
+
+static struct wkup_pin_dt_cfg_t wkup_pins_cfgs[] = {
+	DT_FOREACH_CHILD(RTS5912_ULPM_NODE, WKUP_PIN_CFG_DT_COMMA)};


Use DT_INST_FOREACH_CHILD(0, WKUP_PIN_CFG_DT_COMMA)

keith-zephyr · 2025-03-27T15:24:05Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+/**
+ * @brief Enable VOUT function and set the VOUT default value.
+ */
+void ULPMStart(void)


Use snake-case for C code.
https://docs.zephyrproject.org/latest/contribute/style/code.html

Please fix all functions and variables added in this PR to use snake-case.

keith-zephyr · 2025-03-28T16:43:46Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+	GPIO_Type *gpio_reg = RTS5912_GPIO_REG_BASE;
+	int i, id;
+
+	printk("rts5912 ULPM enabled\n");


Use logging instead of printk throughout the file.

keith-zephyr · 2025-03-28T16:58:54Z

dts/bindings/power/realtek,rts5912-ulpm.yaml

+    wkup-pin-pol-auto:
+      type: boolean
+      description: |
+        True if auto detect current VIN level and set opposite polarity.


I'm not sure of the value of the `wkup-pin-pol-auto' property. The current state of the pin could change depending on the order drivers initialize.

I think it is enough that you include wkup-pin-pol property to explicitly set the polarity.

removed. done.

keith-zephyr · 2025-03-28T17:01:45Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+			if (wkup_pins_cfgs[i].wkup_pin_pol_auto) {
+				/* get current pin state */
+				if (gpio_reg->GCR[RTS5912_VIN_GPIO_INDEX + id] &
+				    GPIO_GCR_PINSTS_Msk) { /* Falling Edge */
+					sys_reg->VIVOCTRL |= BIT(SYSTEM_VIVOCTRL_VIN0POL_Pos + id);
+					printk("Falling Edge\n");
+				} else {
+					/* Rising Edge */
+					sys_reg->VIVOCTRL &= ~BIT(SYSTEM_VIVOCTRL_VIN0POL_Pos + id);
+					printk("Rising Edge\n");
+				}


You shouldn't read the GPIO pin state by directly accessing the register. Instead you should add a gpios property to the node and use gpio_pin_get_raw().

However, I don't think the auto setting is useful, so you could just delete all the GPIO references in this PR.

remove the auto function.

keith-zephyr · 2025-03-28T17:02:05Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+
+#define RTS5912_ULPM_NODE DT_NODELABEL(ulpm)
+
+#define ULPM_RTS5912_MAX_NB_WKUP_PINS DT_INST_PROP(0, wkup_pins_nb)


I suggest renaming the property wkup-pins-max.

keith-zephyr · 2025-03-28T17:07:37Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+#define ULPM_RTS5912_MAX_NB_WKUP_PINS DT_INST_PROP(0, wkup_pins_nb)
+
+#define RTS5912_VIN_GPIO_INDEX 112
+#define RTS5912_SCCON_REG_BASE ((SYSTEM_Type *)(DT_REG_ADDR(DT_NODELABEL(sccon))))


If this driver needs to manipulate the system clock controller, you should add new routines to the clock_control_rts5912_sccon.c driver instead of accessing the registers directly.

That way all register reads and writes to the SCCON are contained in one place.

The system clock control register not only has clock source power controller, it also includes specific controller register such as VIVO , LDO and etc... so, some specific function register won't exposed on clock_control_rts5912_sccon.c

Non-blocking comment:

You could add routines to clock_control_rts5912_sccon.c for reading and writing registers defined by the SYSTEM_Type structure.

uint32_t rts5912_read_sys_reg(int offset); void rts5912_write_sys_reg(int offset, uint32_t write_value);

This avoids each driver having to look up the sccon nodelabel.

Otherwise, you can keep the direct register access to VIVOCTRL in this driver, but you should move the macro RTS5912_SCCON_REG_BASE to reg_system.h so that it can be shared by multiple drivers.

Hi @Titan-Realtek ,

Could you please move RTS5912_SCCON_REG_BASE to reg/reg_system.h to avoid the multiple define and help to check other drivers have used this kind of MACRO.

Thanks!

keith-zephyr · 2025-04-02T21:36:08Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+
+#define ULPM_RTS5912_MAX_NB_WKUP_PINS DT_INST_PROP(0, wkup_pins_max)
+
+#define RTS5912_VIN_GPIO_INDEX 112


This define is no longer used.

keith-zephyr · 2025-04-02T21:40:39Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+#define ULPM_RTS5912_MAX_NB_WKUP_PINS DT_INST_PROP(0, wkup_pins_nb)
+
+#define RTS5912_VIN_GPIO_INDEX 112
+#define RTS5912_SCCON_REG_BASE ((SYSTEM_Type *)(DT_REG_ADDR(DT_NODELABEL(sccon))))


Non-blocking comment:

You could add routines to clock_control_rts5912_sccon.c for reading and writing registers defined by the SYSTEM_Type structure.

uint32_t rts5912_read_sys_reg(int offset); void rts5912_write_sys_reg(int offset, uint32_t write_value);

This avoids each driver having to look up the sccon nodelabel.

Otherwise, you can keep the direct register access to VIVOCTRL in this driver, but you should move the macro RTS5912_SCCON_REG_BASE to reg_system.h so that it can be shared by multiple drivers.

keith-zephyr · 2025-04-08T17:55:26Z

soc/realtek/ec/rts5912/poweroff.c

+#ifdef CONFIG_SOC_RTS5912_ULPM
+	rts5912_ulpm_enable();
+#endif /* CONFIG_SOC_RTS5912_ULPM */


It's preferred to use IS_ENABLED instead of #ifdef when possible.

Suggested change

#ifdef CONFIG_SOC_RTS5912_ULPM

rts5912_ulpm_enable();

#endif /* CONFIG_SOC_RTS5912_ULPM */

if (IS_ENABLED(CONFIG_SOC_RTS5912_ULPM) {

rts5912_ulpm_enable();

}

keith-zephyr · 2025-04-08T17:56:38Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+	/* wake up polarity auto detect and set */
+	bool wkup_pin_pol_auto;


wkup_pin_pol_auto is not used

done: removed.

keith-zephyr · 2025-04-09T15:35:23Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+
+BUILD_ASSERT(DT_NUM_INST_STATUS_OKAY(DT_DRV_COMPAT) <= 1, "Unsupported number of instances");
+
+#define ULPM_SLEEP(x) k_msleep(x)


Minor issue - there is no need to create a macro to wrap k_msleep() - just call this function directly.

Using a macro here makes it harder to understand the code.

keith-zephyr · 2025-04-09T15:37:19Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+	SYSTEM_Type *sys_reg = RTS5912_SCCON_REG_BASE;
+	/* Update Register & reset bit */
+	ULPM_SLEEP(10);
+	sys_reg->VIVOCTRL |= SYSTEM_VIVOCTRL_REGWREN_Msk;
+	ULPM_SLEEP(10);
+	sys_reg->VIVOCTRL &= ~(SYSTEM_VIVOCTRL_REGWREN_Msk);
+	ULPM_SLEEP(10);


This is easier to understand without the ULPM_SLEEP wrapper.

Suggested change

SYSTEM_Type *sys_reg = RTS5912_SCCON_REG_BASE;

/* Update Register & reset bit */

ULPM_SLEEP(10);

sys_reg->VIVOCTRL |= SYSTEM_VIVOCTRL_REGWREN_Msk;

ULPM_SLEEP(10);

sys_reg->VIVOCTRL &= ~(SYSTEM_VIVOCTRL_REGWREN_Msk);

ULPM_SLEEP(10);

SYSTEM_Type *sys_reg = RTS5912_SCCON_REG_BASE;

/* Update Register & reset bit */

k_msleep(10);

sys_reg->VIVOCTRL |= SYSTEM_VIVOCTRL_REGWREN_Msk;

k_msleep(10);

sys_reg->VIVOCTRL &= ~(SYSTEM_VIVOCTRL_REGWREN_Msk);

k_msleep(10);

keith-zephyr · 2025-04-09T15:51:13Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+	/* starts from 0 */
+	uint32_t wkup_pin_id;
+	/* wake up polarity */
+	uint8_t wkup_pin_pol;


Suggested change

uint8_t wkup_pin_pol;

enum wkup_pin_pol pin_pol;

keith-zephyr · 2025-04-09T15:51:31Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+	/* wake up polarity */
+	uint8_t wkup_pin_pol;
+	/* wake up pin mode, VIN / GPIO */
+	uint8_t wkup_pin_mode;


Suggested change

uint8_t wkup_pin_mode;

enum wkup_pin_mode pin_mode;

JasonLin-RealTek · 2025-04-21T13:47:54Z

soc/realtek/ec/rts5912/poweroff.c

+
+void z_sys_poweroff(void)
+{
+	if (IS_ENABLED(CONFIG_SOC_RTS5912_ULPM)) {


Hi @Titan-Realtek ,
If the ulpm device doesn't be enabled in the environment and you call the z_sys_poweroff().
Is the code will stuck here forever?

JasonLin-RealTek · 2025-04-21T13:58:00Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+
+#define WKUP_PIN_NODE_LABEL(i) wkup_pin_##i
+
+#define WKUP_PIN_NODE_ID_BY_IDX(idx) DT_CHILD(STM32_PWR_NODE, WKUP_PIN_NODE_LABEL(idx))


Hi @Titan-Realtek ,
Is STM32_PWR_NODE be nice to appear in here?

Besied this, please check all the Macro and header are used here.

Thanks!

JasonLin-RealTek · 2025-04-21T14:03:00Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+#define ULPM_RTS5912_MAX_NB_WKUP_PINS DT_INST_PROP(0, wkup_pins_nb)
+
+#define RTS5912_VIN_GPIO_INDEX 112
+#define RTS5912_SCCON_REG_BASE ((SYSTEM_Type *)(DT_REG_ADDR(DT_NODELABEL(sccon))))


Hi @Titan-Realtek ,

Could you please move RTS5912_SCCON_REG_BASE to reg/reg_system.h to avoid the multiple define and help to check other drivers have used this kind of MACRO.

Thanks!

Port rts5912 ULPM on Zephyr Signed-off-by: Titan Chen <[email protected]>

Copilot

Pull Request Overview

This PR ports ULPM support for the rts5912 SOC on Zephyr by adding ULPM header and source files, updating register definitions, and integrating the functionality into the poweroff sequence and DTS bindings.

Added ULPM driver header and implementation files for ULPM control and wake-up pin configuration.
Updated system register definitions to accommodate new ULPM settings.
Integrated ULPM trigger in the SOC poweroff path and provided DTS binding for configuration.

Reviewed Changes

Copilot reviewed 5 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
soc/realtek/ec/rts5912/rts5912_ulpm.h	Added header declarations for ULPM enable function.
soc/realtek/ec/rts5912/rts5912_ulpm.c	Added ULPM driver implementation including register and pin setup.
soc/realtek/ec/rts5912/reg/reg_system.h	Updated register definitions to include new VODEF settings.
soc/realtek/ec/rts5912/poweroff.c	Integrated ULPM enable call for SOC poweroff procedure.
dts/bindings/power/realtek,rts5912-ulpm.yaml	Added DTS binding for ULPM configuration.

Files not reviewed (3)

dts/arm/realtek/ec/rts5912.dtsi: Language not supported
soc/realtek/ec/rts5912/CMakeLists.txt: Language not supported
soc/realtek/ec/rts5912/Kconfig: Language not supported

Copilot · 2025-04-24T06:42:23Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+				LOG_DBG("Falling Edge\n");
+			} else {
+				/* Rising Edge */
+				sys_reg->VIVOCTRL &= ~BIT(SYSTEM_VIVOCTRL_VIN0POL_Pos + id);
+				LOG_DBG("Rising Edge\n");


Log messages for wake-up pin edge configuration appear reversed: when the pin polarity is RISING, the subsequent debug log outputs 'Falling Edge'. Please verify and correct the log messages to accurately reflect the hardware configuration.

Suggested change

LOG_DBG("Falling Edge\n");

} else {

/* Rising Edge */

sys_reg->VIVOCTRL &= ~BIT(SYSTEM_VIVOCTRL_VIN0POL_Pos + id);

LOG_DBG("Rising Edge\n");

LOG_DBG("Rising Edge\n");

} else {

/* Rising Edge */

sys_reg->VIVOCTRL &= ~BIT(SYSTEM_VIVOCTRL_VIN0POL_Pos + id);

LOG_DBG("Falling Edge\n");

Copilot · 2025-04-24T06:42:24Z

soc/realtek/ec/rts5912/rts5912_ulpm.c

+void rts5912_ulpm_enable(void)
+{
+	SYSTEM_Type *sys_reg = RTS5912_SCCON_REG_BASE;
+	int i, id;


The variable 'id' is declared as an int but is used to store a value from the unsigned field 'wkup_pin_id'. Consider declaring 'id' as uint32_t to maintain type consistency and ensure proper boundary checks.

Suggested change

int i, id;

int i;

uint32_t id;

zephyrbot added area: Power Management platform: Realtek EC labels Mar 6, 2025

zephyrbot requested review from bjarki-andreasen, ceolin, JasonLin-RealTek, JordanYates, nashif, teburd and tmleman March 6, 2025 11:25

zephyrbot assigned JasonLin-RealTek Mar 6, 2025

Titan-Realtek force-pushed the soc_ulpm_rts5912 branch 3 times, most recently from b606358 to 6f7edd2 Compare March 6, 2025 23:20

nordicjm requested changes Mar 7, 2025

View reviewed changes

Titan-Realtek force-pushed the soc_ulpm_rts5912 branch 3 times, most recently from 9aa63f3 to 7d15a9a Compare March 9, 2025 22:41

Titan-Realtek requested a review from nordicjm March 14, 2025 11:32

nordicjm previously approved these changes Mar 14, 2025

View reviewed changes

keith-zephyr requested changes Mar 27, 2025

View reviewed changes

Titan-Realtek dismissed nordicjm’s stale review via 2798444 March 28, 2025 00:36

Titan-Realtek force-pushed the soc_ulpm_rts5912 branch from 7d15a9a to 2798444 Compare March 28, 2025 00:36

keith-zephyr reviewed Mar 28, 2025

View reviewed changes

Titan-Realtek force-pushed the soc_ulpm_rts5912 branch 3 times, most recently from 10a0b9c to 6aa7001 Compare March 31, 2025 04:55

keith-zephyr reviewed Apr 2, 2025

View reviewed changes

Titan-Realtek force-pushed the soc_ulpm_rts5912 branch from 6aa7001 to 58bb2b8 Compare April 3, 2025 23:25

keith-zephyr reviewed Apr 8, 2025

View reviewed changes

Titan-Realtek force-pushed the soc_ulpm_rts5912 branch from 58bb2b8 to fcaf9e4 Compare April 8, 2025 23:17

keith-zephyr reviewed Apr 9, 2025

View reviewed changes

Titan-Realtek force-pushed the soc_ulpm_rts5912 branch from fcaf9e4 to c4829d8 Compare April 10, 2025 05:28

keith-zephyr previously approved these changes Apr 10, 2025

View reviewed changes

keith-zephyr requested a review from tristan-google April 14, 2025 22:27

JasonLin-RealTek requested changes Apr 21, 2025

View reviewed changes

Titan-Realtek dismissed keith-zephyr’s stale review via c609abc April 22, 2025 03:50

Titan-Realtek force-pushed the soc_ulpm_rts5912 branch 2 times, most recently from c609abc to a0b24de Compare April 22, 2025 03:54

soc : realtek: ec: rts5912: add support ULPM

365d1c5

Port rts5912 ULPM on Zephyr Signed-off-by: Titan Chen <[email protected]>

Titan-Realtek force-pushed the soc_ulpm_rts5912 branch from a0b24de to 365d1c5 Compare April 22, 2025 03:59

keith-zephyr approved these changes Apr 22, 2025

View reviewed changes

JasonLin-RealTek self-requested a review April 24, 2025 02:56

JasonLin-RealTek approved these changes Apr 24, 2025

View reviewed changes

kartben requested a review from Copilot April 24, 2025 06:41

Copilot AI reviewed Apr 24, 2025

View reviewed changes

kartben merged commit e6bb7fc into zephyrproject-rtos:main Apr 24, 2025
24 checks passed


		#define RTS5912_ULPM_NODE DT_NODELABEL(ulpm)

		#define ULPM_RTS5912_MAX_NB_WKUP_PINS DT_INST_PROP(0, wkup_pins_nb)


		#define ULPM_SLEEP(x) k_msleep(x)

		#define RTS5912_ULPM_NODE DT_NODELABEL(ulpm)


		#define ULPM_RTS5912_MAX_NB_WKUP_PINS DT_INST_PROP(0, wkup_pins_max)

		#define RTS5912_VIN_GPIO_INDEX 112

		/* wake up polarity auto detect and set */
		bool wkup_pin_pol_auto;


		BUILD_ASSERT(DT_NUM_INST_STATUS_OKAY(DT_DRV_COMPAT) <= 1, "Unsupported number of instances");

		#define ULPM_SLEEP(x) k_msleep(x)


		#define WKUP_PIN_NODE_LABEL(i) wkup_pin_##i

		#define WKUP_PIN_NODE_ID_BY_IDX(idx) DT_CHILD(STM32_PWR_NODE, WKUP_PIN_NODE_LABEL(idx))

soc : realtek: ec: rts5912: add support ULPM #86717

soc : realtek: ec: rts5912: add support ULPM #86717

Conversation

Titan-Realtek commented Mar 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI Apr 24, 2025

Choose a reason for hiding this comment

Copilot AI Apr 24, 2025

Choose a reason for hiding this comment