timer/t005: Align systimer/physical EL1 timer start and add CNTP_CVAL_EL02 support

[xayahion]

hi @avinaw01-arm

could you please help review this timer-related issue?

Background
While reviewing and running the ACS timer test test_pool/timer/t005.c, I noticed two potential issues that may affect correctness on some platforms, especially under virtualization / E2H configurations.

1.val/src/acs_timer.c : missing E2H branch for programming compare value (CNTP_CVAL_EL02)
In val/src/acs_timer.c (around the implementation of val_timer_set_phy_el1, currently near the referenced line), the code programs the physical timer compare value using CNTP_CVAL_EL0 style accesses.

sysarch-acs/val/src/acs_timer.c

Lines 239 to 241 in 7a82e7e

	case CntpCval:
	write_cntp_cval_el0(*data_buf);
	break;

However, when E2H is enabled, the correct register to program may be CNTP_CVAL_EL02. Without an E2H-specific branch,
ArmArchTimerReadReg(CntpCval) will call read_cntp_cval_el02 but this reg has wrong value. I believe the read and write helpers should be implemented as a pair.

sysarch-acs/val/src/acs_timer.c

Lines 142 to 143 in 7a82e7e

	case CntpCval:
	return effective_e2h ? read_cntp_cval_el02() : read_cntp_cval_el0();

i believe

Suggestion
Add an E2H handling path in val_timer_set_phy_el1() to write CNTP_CVAL_EL02 (e.g., via a write_cntp_cval_el02() helper) when E2H is active.

2.t005.c: systimer and physical EL1 timer are not started at the same time
In t005.c, the system timer is programmed with:

sysarch-acs/test_pool/timer/t005.c

Line 104 in 7a82e7e

val_timer_set_system_timer((addr_t)cnt_base_n, sys_timer_ticks);

and the physical EL1 timer is programmed separately via:

sysarch-acs/test_pool/timer/t005.c

Line 115 in 7a82e7e

val_timer_set_phy_el1(pe_timer_ticks);

These two operations are not placed together, so the two timers may not be armed “simultaneously”. Depending on implementation and execution latency, this can introduce a start-time skew (delay) between the two timers, which may cause intermittent failures or unexpected deltas when the test assumes they start at roughly the same time.

Suggestion
Move val_timer_set_system_timer((addr_t)cnt_base_n, sys_timer_ticks); to be adjacent to val_timer_set_phy_el1(pe_timer_ticks); (i.e., program both timers back-to-back), so they are armed as close in time as possible.

I’d appreciate any feedback on whether the timer arming order and the E2H (CNTP_CVAL_EL02) handling are correct for ACS expectations.
If you agree that the changes are appropriate, please let me know and I can put together a PR/commit for review.

Thanks!

[rajatgoyal47]

Hi @xayahion,

For the first issue, the observation looks valid. In the current code, the read path for CntpCval/CntvCval selects the EL02 register when effective_e2h is set, but the write path always writes to the EL0 register. This causes inconsistent read/write behavior under E2H. We should update the write path to use write_cntp_cval_el02() / write_cntv_cval_el02() when effective_e2h is set.

For the second issue, aligning the system timer and physical EL1 timer programming back-to-back is a valid suggestion. Before raising it as a required fix, could you please confirm whether you are seeing any failures, intermittent deltas, or platform-specific issues with the current timer arming order?

Regards,
ACS Team

[xayahion]

hi @rajatgoyal47
thanks for your reply~

this is my local result for changing systen timer called sequence.

//val_timer_set_system_timer((addr_t)cnt_base_n, sys_timer_ticks); <--- mask this

irq_received = 0;

intid = val_timer_get_info(TIMER_INFO_SYS_INTID, timer_num);
val_gic_install_isr(intid, isr_sys_timer);

intid_phy = val_timer_get_info(TIMER_INFO_PHY_EL1_INTID, 0);
val_gic_install_isr(intid_phy, isr_phy_el1);

/* Start system timer */
val_timer_set_system_timer((addr_t)cnt_base_n, sys_timer_ticks);  <---- move to this
/* Start EL1 PHY timer */
val_timer_set_phy_el1(pe_timer_ticks);

i dont see any failure, the original result i will provide soon.

Selected rules: B_TIME_09

---------------------- Running tests ------------------------

    *** Running TIMER tests ***

B_TIME_09 : 405 : Restore PE timer on PE wake up
	    test_pool/timer/t005.c:350 t005_entry
	       PSCI VERSION = 10001
	       PSCI MAJOR VERSION = 1
	       PSCI FEATURS = 1
	       PSCI PWR_STATE_FMT = 0                
	       System timer interrupt received
   Result:  PASSED
-------------------- Suite run complete --------------------

---------- ACS Summary ----------
   Total Rules Run        : 1
   Passed                 : 1
   Passed (*Partial)      : 0
   Warnings               : 0
   Skipped                : 0
   Failed                 : 0
   PAL Not Supported      : 0
   Test Not Implemented   : 0
---------------------------------

the original result is FAIL

Selected rules: B_TIME_09

---------------------- Running tests ------------------------

    *** Running TIMER tests ***

B_TIME_09 : 405 : Restore PE timer on PE wake up
	    test_pool/timer/t005.c:350 t005_entry

	       System timer interrupt received
	       PSCI VERSION = 10001
	       PSCI MAJOR VERSION = 1
	       PSCI FEATURS = 1
	       PSCI PWR_STATE_FMT = 0                

	       Received el1_phy interrupt   
	ERROR: Failed at checkpoint -  2
   Result:  FAILED
-------------------- Suite run complete --------------------

---------- ACS Summary ----------
   Total Rules Run        : 1
   Passed                 : 0
   Passed (*Partial)      : 0
   Warnings               : 0
   Skipped                : 0
   Failed                 : 1
   PAL Not Supported      : 0
   Test Not Implemented   : 0
---------------------------------

Thanks!

[xayahion]

btw, i have finished a commit for the first issue, could you merge it to main branch?
xayahion@8b2749a
Thanks!

[rajatgoyal47]

Hi @xayahion,

Your changes look great. Could you please also add the changes for cntv_cval and reordering the set timer functions? We can create a pull request for all the changes at once.

Regards,
ACS Team