Rework Xilinx TTC driver #23567

stephanosio · 2020-03-18T14:06:34Z

Rework Xilinx TTC driver to support tickless mode and fix the k_busy_wait hang issue when calling with interrupts locked.

Closes #19869 (tickless support issue)
Fixes #23541

This commit enables the QEMU icount emulation mode for improved timing stability. In normal emulation mode (without icount), the emulation timing of the TTC system timer is particularly unstable and this results in a high CI failure rate. For more details, refer to the issues zephyrproject-rtos#14173 and zephyrproject-rtos#22904. Signed-off-by: Stephanos Ioannidis <root@stephanos.io>

The `tests/kernel/context` test fails for the `qemu_cortex_r5` when the icount emulation mode is used, because the Xilinx QEMU ignores the WFI instruction when the icount parameter is specified. This will be fixed in the Zephyr SDK 0.11.3 and this commit must be reverted once the CI is updated to use this new SDK version. For more details, see zephyrproject-rtos/sdk-ng#191. Signed-off-by: Stephanos Ioannidis <root@stephanos.io>

zephyrbot · 2020-03-18T14:09:15Z

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

andyross

Looks good. Some nitpicking but nothing I can see that should block merge.

drivers/timer/xlnx_psttc_timer.c

andyross · 2020-03-18T16:29:57Z

boards/arm/qemu_cortex_r5/board.cmake

@@ -9,6 +9,7 @@ set(QEMU_CPU_TYPE_${ARCH} cortex-r5)
 set(QEMU_FLAGS_${ARCH}
  -nographic
  -machine arm-generic-fdt
+  -icount shift=3,align=off,sleep=off -rtc clock=vm


No complaints here, though IIRC there are still some issues in the main -icount patch. Does this work for everything?

This change is actually from #22904.

There is an issue with the WFI instruction in the Xilinx QEMU when icount mode is used, but this only affects one Zephyr test (tests/kernel/context) and does not really have a real impact in terms of functionality.

#22904 (comment)

andyross · 2020-03-18T16:31:12Z

tests/kernel/context/testcase.yaml

@@ -2,3 +2,6 @@ tests:
  kernel.common:
    tags: kernel
    min_ram: 20
+    # FIXME: Remove `qemu_cortex_r5` exclusion once Zephyr SDK 0.11.3 is
+    #        mainlined (see #22904).
+    platform_exclude: qemu_cortex_r5


Seems cleaner to me to just rush the SDK release than to commit and revert this.

That would be nice; though, @galak seems to be busy working on something else, so I am not sure if that is feasible.

andyross · 2020-03-18T16:35:11Z

drivers/timer/xlnx_psttc_timer.c

-#else
-#error ("No timer is specified")
-#endif
+#define TICKS_PER_SEC		CONFIG_SYS_CLOCK_TICKS_PER_SEC


Style: unless this is needed to keep line lengths under control, I'd prefer to see the existing kconfig used directly instead of a local synonym.

andyross · 2020-03-18T16:35:53Z

drivers/timer/xlnx_psttc_timer.c

-#error ("No timer is specified")
-#endif
+#define TICKS_PER_SEC		CONFIG_SYS_CLOCK_TICKS_PER_SEC
+#define CYCLES_PER_SEC		TIMER_CLOCK_FREQUECY


Ditto, though I guess DTS constants are really ugly, so I feel less strongly about this one.

yes, they get very long and ugly soon after this.

drivers/timer/xlnx_psttc_timer.c

andyross · 2020-03-18T16:43:26Z

drivers/timer/xlnx_psttc_timer.c

+			CYCLES_NEXT_MAX : CYCLES_PER_TICK;
+	sys_write32(reg_val, TIMER_BASE_ADDR + XTTCPS_MATCH_0_OFFSET);
+
+	/* Connect timer interrupt */


Style: "IRQ_CONNECT(TIMER_IRQ, ..." sufficiently implies that the timer interrupt is connected without the comment. :)

Just wanted to provide a logical flow of this function in the comments (i.e. even without reading actual code, you will know what this function does just only reading comments).

Though, I agree that this would often qualify as a "state-the-obvious" kind of comment :p

drivers/timer/xlnx_psttc_timer.c

drivers/timer/xlnx_psttc_timer_priv.h

This commit updates the 'xlnx_psttc_timer' to implement the current system timer API. Signed-off-by: Stephanos Ioannidis <root@stephanos.io>

The `xlnx,ttcps` binding, despite having the file name of `xlnx,ttcps.yaml`, had the compatible property of `cdns,ttc`. While it is true that the Xilinx ZynqMP platform embeds the Cadence Triple Timer Counter (TTC) IP core, its TTC differs from the original Cadence core in that it implements 32-bit counters, instead of the 16-bit counters defined in the original; hence, the Xilinx variant is not compatible with the original Cadence version and should be treated as a different device. This commit changes the `xlnx,ttcps.yaml` compatible property to `xlnx,ttcps` for the above reasons. Signed-off-by: Stephanos Ioannidis <root@stephanos.io>

This commit reworks the Xilinx TTC timer driver to use the "match" mode instead of the "interval" mode which counts up to the specified value and resets to zero. Using the "match" mode ensures that the timer keeps counting even after an interrupt is triggered, and facilitates the tickless mode support implementation. This also allows `z_timer_cycle_get_32` to return the correct cycle count when interrupt is locked; thereby, fixing the k_busy_wait hang issue. Note that the TTC "match" mode emulation (and tickless timer operation) is only stable when the QEMU icount mode is enabled. Signed-off-by: Stephanos Ioannidis <root@stephanos.io>

carlescufi

LGTM

stephanosio added 2 commits March 18, 2020 21:52

stephanosio requested review from andrewboie, andyross, galak, nashif and wjliang as code owners March 18, 2020 14:06

stephanosio added area: Drivers area: Timer Timer platform: Xilinx Xilinx labels Mar 18, 2020

zephyrbot added area: Devicetree area: Boards area: Tests Issues related to a particular existing or missing test area: Kernel labels Mar 18, 2020

stephanosio mentioned this pull request Mar 18, 2020

xilinx_zynqmp: k_busy_wait() never returns if called with interrupts locked #23541

Closed

stephanosio force-pushed the xlnx_ttc_new branch from 417b333 to c9067fe Compare March 18, 2020 14:15

stephanosio added this to the v2.3.0 milestone Mar 18, 2020

andyross approved these changes Mar 18, 2020

View reviewed changes

stephanosio added 3 commits March 19, 2020 10:39

drivers: timer: xlnx_psttc_timer: Use the current system timer API

5da7eac

This commit updates the 'xlnx_psttc_timer' to implement the current system timer API. Signed-off-by: Stephanos Ioannidis <root@stephanos.io>

stephanosio force-pushed the xlnx_ttc_new branch from c9067fe to 7a17804 Compare March 19, 2020 03:08

carlescufi approved these changes Mar 19, 2020

View reviewed changes

This was referenced Mar 20, 2020

Support nested interrupts on AArch32 (non-Cortex-M) #23636

Merged

doc: enforce minimum python version #23647

Merged

nashif merged commit fc941d5 into zephyrproject-rtos:master Mar 21, 2020

stephanosio deleted the xlnx_ttc_new branch April 23, 2020 06:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework Xilinx TTC driver #23567

Rework Xilinx TTC driver #23567

stephanosio commented Mar 18, 2020

zephyrbot commented Mar 18, 2020 •

edited

Loading

andyross left a comment

andyross Mar 18, 2020

stephanosio Mar 19, 2020

andyross Mar 18, 2020

stephanosio Mar 19, 2020

andyross Mar 18, 2020

andyross Mar 18, 2020

stephanosio Mar 19, 2020

andyross Mar 18, 2020

stephanosio Mar 19, 2020

carlescufi left a comment

Rework Xilinx TTC driver #23567

Rework Xilinx TTC driver #23567

Conversation

stephanosio commented Mar 18, 2020

zephyrbot commented Mar 18, 2020 • edited Loading

andyross left a 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

carlescufi left a comment

Choose a reason for hiding this comment

zephyrbot commented Mar 18, 2020 •

edited

Loading