cortex-m: Add initial dwt and dcb support #3246

codingHahn · 2022-09-27T14:17:40Z

Pull Request Overview

The DWT and DCB peripherals are used to access various debug functionality in the chip, such as various counters.
This commit adds register descriptions for most of the DWT and some of the DCB. It also adds a very simple capsule that allows reading the cycle counter from userspace.

Testing Strategy

This pull request was tested by getting the cycle count and it checks out.

TODO or Help Wanted

This pull request still needs the rest of the registers of some peripherals.

The capsule needs to be added to every supported board and could be expanded to grant access to the other capabilities of the peripherals. We also need to consider security implications for allowing debug peripherals in userspace.

Documentation Updated

Updated the relevant files in /docs, or no updates are required.

Formatting

Ran make prepush.

The DWT and DCB peripherals are used to access various debug functionality in the chip, such as various counters. This commits adds register descriptions for most of DWT and some of DCB. It also adds a very simple capsule that allows reading the cycle counter from userspace.

arch/cortex-m/src/dcb.rs

arch/cortex-m/src/dwt.rs

bradjc · 2022-09-27T14:26:46Z

arch/cortex-m/src/dwt.rs

+const DWT: StaticRef<DwtRegisters> = unsafe { StaticRef::new(0xE0001000 as *const DwtRegisters) };
+
+/// Enable the cycle counter
+/// [`DCB::enable_debug_and_trace()`] needs to be enabled for this to work


Why not just do that then?

bradjc · 2022-09-27T14:27:30Z

capsules/Cargo.toml

@@ -8,3 +8,4 @@ edition = "2021"
 kernel = { path = "../kernel" }
 enum_primitive = { path = "../libraries/enum_primitive" }
 tickv = { path = "../libraries/tickv" }
+cortexm = { path = "../arch/cortex-m" }


We cannot make capsules dependent on something chip specific.

Instead, we have to use a HIL to abstract HW from capsules.

Ok, I'll take a look and amend the pr

ppannuto · 2022-09-28T17:44:56Z

arch/cortex-m/src/dcb.rs

+use kernel::utilities::registers::{register_bitfields, register_structs, ReadWrite};
+use kernel::utilities::StaticRef;
+
+//TODO: Rest of the Registers


There's only three more here; I think we'd ask for the register definition to be fully implemented before pulling this in (i.e., flesh out the rest of register_structs and register bitfields; don't need all the getters/setters/syscalls at first).

arch/cortex-m/src/dcb.rs

ppannuto · 2022-09-28T17:48:06Z

arch/cortex-m/src/dcb.rs

+register_bitfields![u32,
+    DebugExceptionAndMonitorControl [
+        /// Write 1 to globally enable all DWT and ITM features.
+        /// RW.


This whole register is declared ReadWrite already; are the RO/WO fields? If not, these RWs should probably be removed.

ppannuto · 2022-09-28T17:51:17Z

arch/cortex-m/src/dcb.rs

+/// Enable the Debug and Trace unit `DWT`
+/// This has to be enabled before using any feature of the `DWT`
+pub fn enable_debug_and_trace() {
+    DCB.demcr
+        .modify(DebugExceptionAndMonitorControl::TRCENA::SET);
+}
+
+/// Disable the Debug and Trace unit `DWT`
+pub unsafe fn disable_debug_and_trace() {
+    DCB.demcr
+        .modify(DebugExceptionAndMonitorControl::TRCENA::CLEAR);


Consider wrapping DWT in a PeripheralManager instance (e.g.

tock/chips/sam4l/src/i2c.rs

Line 569 in 7fcb766

impl PeripheralManagement<TWIMClock> for I2CHw {

) to automate these calls and ensure enable-before-access.

bradjc · 2022-09-29T21:52:36Z

kernel/src/hil/debug.rs

+/// Interface for interacting with debug hardware integrated in various SoCs.
+/// Currently allows reading the cycle counter and can be expanded to allow
+/// access to other features in the future.


Suggested change

/// Interface for interacting with debug hardware integrated in various SoCs.

/// Currently allows reading the cycle counter and can be expanded to allow

/// access to other features in the future.

//! Interface for interacting with debug hardware integrated in various SoCs.

//!

//! Currently allows reading the cycle counter.

bradjc · 2022-09-29T21:53:10Z

kernel/src/hil/debug.rs

+use crate::ErrorCode;
+
+pub trait PerformanceCounters {
+    /// Enable the cycle counter. Returns an error, if the cycle counter is not present.


Which error?

bradjc · 2022-09-29T21:53:26Z

kernel/src/hil/debug.rs

+/// access to other features in the future.
+use crate::ErrorCode;
+
+pub trait PerformanceCounters {


Suggested change

pub trait PerformanceCounters {

pub trait CycleCounter {

?

bradjc · 2022-09-29T21:54:34Z

kernel/src/hil/debug.rs

@@ -0,0 +1,15 @@
+/// Interface for interacting with debug hardware integrated in various SoCs.


"debug.rs" is somewhat generic, and there is the potential for confusion with kernel/src/debug.rs. Is there a name that would be somewhat more informative? Maybe hw_debug?

bradjc · 2022-09-29T21:55:10Z

capsules/src/perf.rs

@@ -0,0 +1,51 @@
+//! Provides a performance counter interface for userspace.


perf.rs is too short. If this is for cycle counts, let's name it that.

hudson-ayers

I would love to see DWT support merged, I end up using some modified version of what Amit wrote for the PowerClocks paper all the time when taking performance measurements, and its always a pain to have to throw it somewhere in the kernel.

While I don't think this PR has to be updated to include this, I do think it would be really nice to see this functionality added to kernel/src/debug.rs (similar to debug!() and debug_gpio!()) so that it is easy to take performance measurements from anywhere. This would require some sort of global that boards would initialize with the appropriate implementation of the HIL, or perhaps we could configure it using KernelResources

hudson-ayers · 2022-10-04T20:58:57Z

arch/cortex-m/src/dwt.rs

+        /*
+        // TODO: register_bitfields
+        // Peripheral Identification Register 4
+        (0xFD0 => pid4: ReadOnly<u32, Pid4::Register>),
+
+        // Peripheral Identification Register 5
+        (0xFD4 => pid5: ReadOnly<u32, Pid5::Register>),
+
+        // Peripheral Identification Register 6
+        (0xFD8 => pid6: ReadOnly<u32, Pid6::Register>),
+
+        // Peripheral Identification Register 7
+        (0xFDC => pid7: ReadOnly<u32, Pid7::Register>),
+
+        // Peripheral Identification Register 0
+        (0xFE0 => pid0: ReadOnly<u32, Pid0::Register>),
+
+        // Peripheral Identification Register 1
+        (0xFE4 => pid1: ReadOnly<u32, Pid1::Register>),
+
+        // Peripheral Identification Register 2
+        (0xFE8 => pid2: ReadOnly<u32, Pid2::Register>),
+
+        // Peripheral Identification Register 3
+        (0xFEC => pid3: ReadOnly<u32, Pid3::Register>),
+
+        // Component Identification Register 0
+        (0xFF0 => cid0: ReadOnly<u32, Cid0::Register>),
+
+        // Component Identification Register 1
+        (0xFF4 => cid1: ReadOnly<u32, Cid1::Register>),
+


Lets just delete the unused peripheral identification registers unless you anticipate needing those for something in the short term.

hudson-ayers · 2022-10-04T21:01:36Z

arch/cortex-m/src/dwt.rs

+}
+
+impl hil::debug::PerformanceCounters for Dwt {
+    fn enable_cycle_counter() -> Result<(), ErrorCode> {


Personally I would call this start_cycle_counter() since it actually starts it counting up, rather than just enabling the functionality. Same for disable_cycle_counter() -- I would just call that stop_cycle_counter(), and not have it disable the DCB.

hudson-ayers · 2022-10-04T21:03:51Z

arch/cortex-m/src/dwt.rs

+    }
+}
+
+impl hil::debug::PerformanceCounters for Dwt {


I think it would also be helpful to add a reset_cycle_counter() method to this HIL to reset the counter to 0. This saves from having to get the current count and then perform subtraction, and it just an easy interface (imo). Amit wrote a rough DWT implementation a few years ago that I often use when benchmarking, and it just does the following:

pub fn reset_timer() { DEMCR.set(DEMCR.get() | 0x01000000); DWT.cycnt.set(0); // reset the counter DWT.ctrl.set(0); // disable counter; }

Keeping in mind that on something like the nrf52 this counter will overflow in 67 seconds, it can actually be pretty helpful to be able to reset to 0 rather than have to consider the (somewhat high) possibility of overflow.

hudson-ayers · 2022-10-04T21:05:10Z

arch/cortex-m/src/dwt.rs

+            Err(ErrorCode::NOSUPPORT)
+        }
+    }
+


One other useful function from Amit's old DWT implementation was bench() which could be used to easily benchmark in-kernel code:

pub unsafe fn bench<F: FnOnce()>(f: F) -> u32 { reset_timer(); start_timer(); f(); stop_timer(); get_time() }

I think it would be nice to see something like this included here.

hudson-ayers

@codingHahn will you have a chance to revisit this? If not, I will try to find time to update this with the various suggestions

codingHahn · 2022-12-18T00:48:14Z

Yeah, sorry. I finished my bachelors thesis and had to settle in to working a full time job. I'd love to apply all your suggestions. Problem is that I don't have the board anymore because it belonged to my employer.

hudson-ayers · 2024-05-17T16:30:41Z

Replaced by #3934

bradjc requested changes Sep 27, 2022

View reviewed changes

codingHahn added 2 commits September 28, 2022 14:48

Fix typo and update header

410ddb1

Enable dcb automatically when enabling cycle counter

d96c55e

ppannuto requested changes Sep 28, 2022

View reviewed changes

wip: Use hil

b35e373

github-actions bot added the HIL This affects a Tock HIL interface. label Sep 29, 2022

bradjc requested changes Sep 29, 2022

View reviewed changes

hudson-ayers previously approved these changes Oct 4, 2022

View reviewed changes

hudson-ayers reviewed Dec 9, 2022

View reviewed changes

cortex-m: Begin to add more debug registers

271863c

codingHahn dismissed hudson-ayers’s stale review via 271863c December 18, 2022 11:41

hudson-ayers added the waiting-on-author label Jan 25, 2023

hudson-ayers removed the waiting-on-author label Mar 15, 2024

hudson-ayers self-assigned this Mar 15, 2024

hudson-ayers mentioned this pull request Mar 23, 2024

cortex-m: Add initial dwt and dcb support, take 2 #3934

Merged

2 tasks

hudson-ayers closed this May 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cortex-m: Add initial dwt and dcb support #3246

cortex-m: Add initial dwt and dcb support #3246

codingHahn commented Sep 27, 2022

bradjc Sep 27, 2022

bradjc Sep 27, 2022

codingHahn Sep 28, 2022

ppannuto Sep 28, 2022

ppannuto Sep 28, 2022

ppannuto Sep 28, 2022

bradjc Sep 29, 2022

bradjc Sep 29, 2022

bradjc Sep 29, 2022

bradjc Sep 29, 2022

bradjc Sep 29, 2022

hudson-ayers left a comment

hudson-ayers Oct 4, 2022

hudson-ayers Oct 4, 2022

hudson-ayers Oct 4, 2022

hudson-ayers Oct 4, 2022

hudson-ayers left a comment

codingHahn commented Dec 18, 2022

hudson-ayers commented May 17, 2024

		@@ -0,0 +1,15 @@
		/// Interface for interacting with debug hardware integrated in various SoCs.

		@@ -0,0 +1,51 @@
		//! Provides a performance counter interface for userspace.

cortex-m: Add initial dwt and dcb support #3246

cortex-m: Add initial dwt and dcb support #3246

Conversation

codingHahn commented Sep 27, 2022

Pull Request Overview

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

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

hudson-ayers 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

hudson-ayers left a comment

Choose a reason for hiding this comment

codingHahn commented Dec 18, 2022

hudson-ayers commented May 17, 2024