WCH: ch32v interrupt/timer fixes #91966

petejohanson · 2025-06-20T22:34:07Z

The following were some baseline core fixes needed to get more of the kernel test suite passing on WCH. I'm testing locally on v305, so if others could test this on v307, etc I would appreciate it.

Summary of changes:

I built on the great work in soc: wch: work-around a bug in wfi in the QingKe V2A #88548 with a few tweaks. Namely, I moved this to a common place to be used by more targets, and more importantly, added a select for RISCV_ALWAYS_SWITCH_THROUGH_ECALL which is required in order for the PFIC not to get wedged as soon as we do a switch. Without this selected, the PFIC will stop triggering any interrupts. You could see this manifested in the timer tests, for instance, where the timer ISR would happily trigger until the first call to sleep, which caused a switch, and the ISR never fired again after that.
I've made some minor improvements to the system timer to improve the reliability, mostly around using a lock in the IRS and sys_clock_cycle_get_32 to ensure we don't get ordering issues with the changes to ch32v00x_systick_count and SYSTICK->CNT causing "missed cycles". I've further adjusted the system timer to be properly monotonic, report 64-bit cycles.

Big thanks to @nzmichaelh for the work on #88548. I kept their Signed-off-by intact, and amended the commit to add me as a co-author there. Hopefully that's ok with all parties involved. If we decide to use this PR, this would supersede #88548.

soc/wch/ch32v/qingke_v4f/vector.S

nzmichaelh · 2025-06-21T10:01:51Z

drivers/timer/wch_systick_ch32v00x.c

@@ -27,20 +26,36 @@

 #define SYSTICK ((SysTick_Type *)(DT_INST_REG_ADDR(0)))

+static struct k_spinlock lock;


nit: the Zephyr style is verbose, and puts the local file prefix on everything including statics.

So s/lock/ch32v00x_systick_lock/

Alternatively, as there are now more than one field, put this in a struct ch32v00x_systick_data and have one of those.

nzmichaelh · 2025-06-21T10:03:54Z

drivers/timer/wch_systick_ch32v00x.c

 static volatile uint32_t ch32v00x_systick_count;

 static void ch32v00x_systick_irq(const void *unused)
 {
 	ARG_UNUSED(unused);

+	k_spinlock_key_t key = k_spin_lock(&lock);
+
+	SYSTICK->CTLR &= ~(STK_STE);


Will this cause the system to loose ticks over time?

Addressed this by moving to keeping the timer running, and updating CMP as we go. Passing tests, including the nasty starve test:

*** Booting Zephyr OS build 7335d9637931 *** Running TESTSUITE starve_fn =================================================================== START - test_starve Cycle clock runs at 96000000 Hz There are 960000 cycles per tick (100 Hz ticks) [ 0.020] 1: still running, would pass at 3600 s [ 60.090] 601: still running, would pass at 3600 s [ 120.060] 1200: still running, would pass at 3600 s [ 180.040] 1799: still running, would pass at 3600 s [ 240.020] 2398: still running, would pass at 3600 s [ 300.090] 2998: still running, would pass at 3600 s [ 360.070] 3597: still running, would pass at 3600 s [ 420.050] 4196: still running, would pass at 3600 s [ 480.030] 4795: still running, would pass at 3600 s [ 540.100] 5395: still running, would pass at 3600 s [ 600.080] 5994: still running, would pass at 3600 s [ 660.060] 6593: still running, would pass at 3600 s [ 720.040] 7192: still running, would pass at 3600 s [ 780.020] 7791: still running, would pass at 3600 s [ 840.090] 8391: still running, would pass at 3600 s [ 900.070] 8990: still running, would pass at 3600 s [ 960.050] 9589: still running, would pass at 3600 s [ 1020.030] 10188: still running, would pass at 3600 s [ 1080.020] 10787: still running, would pass at 3600 s [ 1140.100] 11387: still running, would pass at 3600 s [ 1200.090] 11986: still running, would pass at 3600 s [ 1260.070] 12585: still running, would pass at 3600 s [ 1320.060] 13184: still running, would pass at 3600 s [ 1380.040] 13783: still running, would pass at 3600 s [ 1440.030] 14382: still running, would pass at 3600 s [ 1500.010] 14981: still running, would pass at 3600 s [ 1560.100] 15581: still running, would pass at 3600 s [ 1620.090] 16180: still running, would pass at 3600 s [ 1680.070] 16779: still running, would pass at 3600 s [ 1740.060] 17378: still running, would pass at 3600 s [ 1800.040] 17977: still running, would pass at 3600 s [ 1860.030] 18576: still running, would pass at 3600 s [ 1920.010] 19175: still running, would pass at 3600 s [ 1980.100] 19775: still running, would pass at 3600 s [ 2040.080] 20374: still running, would pass at 3600 s [ 2100.070] 20973: still running, would pass at 3600 s [ 2160.050] 21572: still running, would pass at 3600 s [ 2220.040] 22171: still running, would pass at 3600 s [ 2280.030] 22770: still running, would pass at 3600 s [ 2340.010] 23369: still running, would pass at 3600 s [ 2400.100] 23969: still running, would pass at 3600 s [ 2460.080] 24568: still running, would pass at 3600 s [ 2520.070] 25167: still running, would pass at 3600 s [ 2580.050] 25766: still running, would pass at 3600 s [ 2640.040] 26365: still running, would pass at 3600 s [ 2700.020] 26964: still running, would pass at 3600 s [ 2760.010] 27563: still running, would pass at 3600 s [ 2820.100] 28163: still running, would pass at 3600 s [ 2880.080] 28762: still running, would pass at 3600 s [ 2940.070] 29361: still running, would pass at 3600 s [ 3000.050] 29960: still running, would pass at 3600 s [ 3060.040] 30559: still running, would pass at 3600 s [ 3120.020] 31158: still running, would pass at 3600 s [ 3180.010] 31757: still running, would pass at 3600 s [ 3240.090] 32357: still running, would pass at 3600 s [ 3300.080] 32956: still running, would pass at 3600 s [ 3360.060] 33555: still running, would pass at 3600 s [ 3420.050] 34154: still running, would pass at 3600 s [ 3480.040] 34753: still running, would pass at 3600 s [ 3540.020] 35352: still running, would pass at 3600 s [ 3600.010] 35951: still running, would pass at 3600 s [ 3601.010] 35960: Completed 35960 iters without failure PASS - test_starve in 21.869 seconds =================================================================== TESTSUITE starve_fn succeeded ------ TESTSUITE SUMMARY START ------ SUITE PASS - 100.00% [starve_fn]: pass = 1, fail = 0, skip = 0, total = 1 duration = 21.869 seconds - PASS - [starve_fn.test_starve] duration = 21.869 seconds ------ TESTSUITE SUMMARY END ------ =================================================================== RunID: 6832bb16d4f42018061605b673f01a88 PROJECT EXECUTION SUCCESSFUL

nzmichaelh · 2025-06-21T10:05:49Z

soc/wch/ch32v/qingke_v4b/Kconfig

@@ -8,3 +8,6 @@ config SOC_SERIES_QINGKE_V4B
 	select RISCV_ISA_EXT_C
 	select RISCV_ISA_EXT_ZICSR
 	select RISCV_ISA_EXT_ZIFENCEI
+	select RISCV_ALWAYS_SWITCH_THROUGH_ECALL


This is needed on the v2a, but we didn't see the same lockup on the v2c or v4f. Could you confirm that this is needed?

I've verified on v4f (CH32V305), but don't have v2c on hand to verify. Would appreciate help testing, otherwise I can remove this from there for now.

nzmichaelh · 2025-06-21T10:06:47Z

soc/wch/ch32v/qingke_v4f/vector.S

+	csrw    0xbc0, a0
+	li      a0, 0x1e
+	csrw    0x804, a0
+	la t0, _irq_vector_table /* Load address of interrupt vector table */


nit: add tabs to align the operands to match the lines above

nzmichaelh · 2025-06-21T10:07:50Z

soc/wch/ch32v/qingke_v4f/vector.S

+	li      a0, 0x1e
+	csrw    0x804, a0
+	la t0, _irq_vector_table /* Load address of interrupt vector table */
+	ori t0, t0, 0x03
 	csrw	mtvec, a0


Just to double check: on the v2a and v2c, mtvec must be aligned to a 1 KiB boundrary else it will halt. Does the v4f support looser alignments?

I am actually going to revert this particular bit. I plan to work on support for flashing with different offsets to support using tinyuf2, but don't need to wrap that work into this PR.

nzmichaelh · 2025-06-21T10:11:42Z

drivers/timer/wch_systick_ch32v00x.c

@@ -52,13 +67,15 @@ static int ch32v00x_systick_init(void)
 {
 	IRQ_CONNECT(DT_INST_IRQN(0), 0, ch32v00x_systick_irq, NULL, 0);

+	SYSTICK->CTLR = 0;


Note that the reset state of CTLR is zero. This change is fine, but we generally don't do redundant writes like this. It implies that the reset state was somehow unsuitable.

nzmichaelh · 2025-06-21T10:16:52Z

drivers/timer/wch_systick_ch32v00x.c

+
+	k_spinlock_key_t key = k_spin_lock(&lock);
+
+	ret = ch32v00x_systick_count + SYSTICK->CNT;


Note that time can still jump backwards with this code, as CNT may match, get reset to zero, and ch32v00x_systick_count hasn't been updated.

Good news is the problem already exists :)

If you want, you could change the counter to not reset on match, but instead increase STK_CMPR by CYCLES_PER_TICK on each interrupt.

nzmichaelh · 2025-06-21T10:18:27Z

drivers/timer/wch_systick_ch32v00x.c

@@ -10,12 +10,11 @@
 #include <zephyr/drivers/timer/system_timer.h>


nit: in the comment subject line, be more specific about what the fixes are

For example: "drivers: timer: fix a race in the WCH systick time monoticity"

drivers/clock_control/clock_control_wch_rcc.c

nzmichaelh · 2025-06-21T10:26:45Z

drivers/clock_control/clock_control_wch_rcc.c

@@ -154,6 +154,7 @@ static int clock_control_wch_rcc_init(const struct device *dev)
 			RCC->CFGR0 |= RCC_PLLSRC;


A suggestion for the subject line: "drivers: clock_control: disable the HSI PLL prescaler"

That describes what the change does, and the body can describe why such as "For consistency with other SoCs in the family, ..."

Note that this PR does change behaviour. Do a quick grep of the current boards and see if anyone is using the hsi.

Only one using it seems to be ch32v003f4p6_dev_board, which as you noted, doesn't have this prescalar. I've added that preprocessor conditional to catch that.

The RISC-V Machine-Level ISA section 3.3.3 says that `wfi` will complete even if interrupts are masked, but the QingKe V2A does not do this. Work-around by enabling interrupts first. This is the same mitigation as used for the Nordic VPR. Signed-off-by: Michael Hope <michaelh@juju.nz> Co-authored-by: Pete Johanson <peter@peterjohanson.com>

To ensure proper monotonic system time, don't rely on SysTick reset, instead increasing the compare value as needed to trigger the next interrupt/tick. Update to support 64-bit cycles as well. Signed-off-by: Peter Johanson <peter@peterjohanson.com>

Ensure our clock isn't divided when using HSI. Signed-off-by: Peter Johanson <peter@peterjohanson.com>

sonarqubecloud · 2025-06-22T06:40:23Z

Quality Gate passed

Issues
0 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: Interrupt Controller area: Timer Timer platform: WinChipHead area: Clock Control labels Jun 20, 2025

github-actions bot requested review from andyross, dcpleung, nashif, nordic-krch, nzmichaelh, teburd, VynDragon and ycsin June 20, 2025 22:35

github-actions bot assigned nzmichaelh Jun 20, 2025

petejohanson force-pushed the ch32v-v305-and-various-fixes branch 3 times, most recently from 7663891 to f8d1ab6 Compare June 20, 2025 22:50

petejohanson mentioned this pull request Jun 21, 2025

drivers: gpio: add interrupt support for the CH32V family #89139

Merged

petejohanson force-pushed the ch32v-v305-and-various-fixes branch from f8d1ab6 to 177e29d Compare June 21, 2025 00:30

kartben reviewed Jun 21, 2025

View reviewed changes

soc/wch/ch32v/qingke_v4f/vector.S Show resolved Hide resolved

nzmichaelh requested changes Jun 21, 2025

View reviewed changes

petejohanson force-pushed the ch32v-v305-and-various-fixes branch 3 times, most recently from 58fb3d3 to 7874206 Compare June 22, 2025 06:14

petejohanson added 2 commits June 22, 2025 00:15

drivers: clock_control: disable the HSI PLL prescaler

3901514

Ensure our clock isn't divided when using HSI. Signed-off-by: Peter Johanson <peter@peterjohanson.com>

petejohanson force-pushed the ch32v-v305-and-various-fixes branch from 7874206 to 3901514 Compare June 22, 2025 06:15

		@@ -27,20 +26,36 @@

		#define SYSTICK ((SysTick_Type *)(DT_INST_REG_ADDR(0)))

		static struct k_spinlock lock;


		k_spinlock_key_t key = k_spin_lock(&lock);

		ret = ch32v00x_systick_count + SYSTICK->CNT;

		@@ -10,12 +10,11 @@
		#include <zephyr/drivers/timer/system_timer.h>

		@@ -154,6 +154,7 @@ static int clock_control_wch_rcc_init(const struct device *dev)
		RCC->CFGR0 \|= RCC_PLLSRC;

WCH: ch32v interrupt/timer fixes #91966

Are you sure you want to change the base?

WCH: ch32v interrupt/timer fixes #91966

Conversation

petejohanson commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 22, 2025

Quality Gate passed

Uh oh!

Uh oh!

petejohanson commented Jun 20, 2025 •

edited

Loading