Replace queue-based interrupts with NVIC regs #687

alevy · 2017-11-15T23:41:42Z

Pull Request Overview

Instead of using a global queue to notify the event loop about pending
interrups, this commit uses the built in NVIC pending bits themselves.
This has a few benefits:

It removes controller specific ISRs that do nothing but enqueue an interrupt event.
It makes this code generic for all Cortex-M4 chips (and should be ported to Cortex-M0 et
al). As a result of both of these, it removes about 10kB of code on hail
and imix.
it removes the need for controller drivers to handle
enabling/disabling nvic lines themselves.
it's much faster since enqueing and dequeuing in the ring buffer is actually pretty expensive.

Downsides?

The nvic indexes can't be an enum anymore since those "names" are chip specific. Oh well...

Testing Strategy

In principle, this should be a pretty transparent change except some drivers relied on explicit nvic handling to disable interrupts. I believe I've fixed that in all cases, and have tested a number of the example apps on hail and imix. However, more comprehensive testing would be useful.

Tested fairly extensively on Hail, NRF51-DK and NRF52-DK. On Hail, we've run a bunch of apps, including tests/hail, rot13, test/nonvolatile_storage, blink and on the NRFs we ran ble_advertising, blink, tests/aes.

TODO or Help Wanted

The flash driver has a particular place where it actually called the ISR explicitly from the code. That ISR no longer exists (and either way would be useless) and I don't understand what role exactly that was playing, or how to reproduce that functionality (perhaps by just calling handle_interrupt, but why did the original code not just do that?).

~~I left a TODO where that code used to be. Does anyone know how to trigger the issue that led to that hack?~~

Documentation Updated

~~Kernel: The relevant files in /docs have been updated or no updates are required.~~
~~Userland: The application README has been added, updated, or no updates are required.~~

Formatting

make formatall has been run.

bradjc

And of course the issues with flash discussed on irc.

bradjc · 2017-11-16T22:54:42Z

arch/cortex-m4/src/nvic.rs

@@ -0,0 +1,110 @@
+//! Cortex-M3 NVIC


Cortext-M4?

That's just... like... your opinion man

bradjc · 2017-11-17T01:05:12Z

Oh I also tested with the blink, hail, and rot13 apps all running concurrently and they seemed to work.

ppannuto · 2017-11-20T22:31:48Z

arch/cortex-m4/src/lib.rs

+
+    /* Set thread mode to privileged */
+    mov r0, #0
+    msr CONTROL, r0


generic_isr is only called to service interrupts, correct? My reading of B1.3.1 of the ARM v7m ARM is that the processor should be in Handler mode, which is by definition already privileged. I don't think this is needed.

This is needed to make sure we're in privileged mode when we return the kernel, not for this handler's code.

ppannuto · 2017-11-20T22:32:31Z

arch/cortex-m4/src/lib.rs


-    ldr r1, =INTERRUPT_TABLE
-    ldr r0, [r1, r0]
+    /* r1 = NVIC.ICER[r0 / 32] */


From this comment, I expect one of the instructions below to be a memory read?

OK, it's taking an address: r1 = &NVIC.ICER[r0 / 32]

ppannuto · 2017-11-20T22:33:59Z

arch/cortex-m4/src/lib.rs

+    movt r1, #0xe000
+    lsr r2, r0, #5
+    mov r3, #4
+    mul r2, r3


Why load 4 into a register and multiply instead of just shifting again here?

cause like... I don't know how to do that... would it just be lsr r2, r0, #7?

ppannuto · 2017-11-20T22:39:54Z

arch/cortex-m4/src/lib.rs

+    /* r2 = 1 << (r0 & 31) */
+    mov r2, #1
+    mov r3, #31
+    and r3, r0


You can consolidate these two instructions to just
and r3, r0, #31

ppannuto · 2017-11-20T22:41:12Z

arch/cortex-m4/src/nvic.rs

+use kernel::common::volatile_cell::VolatileCell;
+
+#[repr(C, packed)]
+// Registers for the NVIC. Each


Each what? Don't leave me hanging! :D

You get the answer when you buy the DLC

alevy · 2017-11-21T01:28:47Z

Rebased, fixedup, ported to NRF52, cortex-m0/nrf51, but need to test after the rebase and some cleanup.

Last I tested thoroughly before the rebase, sam4l was working as was nrf52. More tomorrow

niklasad1 · 2017-11-21T18:01:23Z

Looks good, have you tested it on both nRF51-DK and nRF52-DK?

alevy · 2017-11-21T18:26:29Z

@niklasad1 literally just tested right now, and after some fixes, it's all working.

The one thing i noticed is that ble_passive_scan seems to block after a few scans, but that is happening on master as well.

alevy · 2017-11-21T18:36:57Z

I think this is ready, maybe it could use some more assembly auditing from the venerable @ppannuto. I trust the assembly insofar as I've tested it, but @ppanuto's caught obvious things in the past that I've missed, and is also good at asking for the right documentation for assembly.

bradjc

Didn't look super thoroughly, just at the parts I've been looking at.

bradjc · 2017-11-21T23:39:59Z

arch/cortex-m4/src/nvic.rs

Gah! Too many files!!!

bradjc · 2017-11-21T23:41:10Z

boards/hail/src/main.rs

We don't want this in Hail proper.

bradjc · 2017-11-21T23:43:53Z

chips/sam4l/src/flashcalw.rs

Don't need this

OK, removed it and I trust you, but I needed it before your changes on master, so should test this just in case before merging (which I can do first thing tomorrow)

Tested. Works.

Instead of using a global queue to notify the event loop about pending interrups, this commit uses the built in NVIC pending bits themselves. This has a few benefits. First, it removes controller specific ISRs that do nothing but enqueue an interrupt event. Second, it makes this code generic for all Cortex-M4 chips (and should be ported to Cortex-M0 et al). As a result of both of these, it removes about 10kB of code on hail and imix. Third, it removes the need for controller drivers to handle enabling/disabling nvic lines themselves. Fourth, it's much faster since enqueing and dequeuing in the ring buffer is actually pretty expensive.

ppannuto

Just looked over the assembly

ppannuto · 2017-11-22T02:37:58Z

arch/cortex-m0/src/lib.rs

Is this comment still appropriate? (also weird spacing for this block?)

ppannuto · 2017-11-22T02:49:07Z

arch/cortex-m0/src/lib.rs

Shift right 31? Is this supposed to be the r0 / 32 step? I'd expect that to be a shift right of 5 bits?

I think the readability here in general could be improve with some pedantry for commenting assembly, i.e.
asrs r3, r0, #5 /* r3 = r0 / 32 */
where every step lists its purpose. Assembly arithmetic is hard enough to justify this

ppannuto · 2017-11-22T03:50:43Z

arch/cortex-m4/src/lib.rs

+    /* NVIC.ICER[r0 / 32] = 1 << (r0 & 31) */
+	movs r2, #1
+	movs r3, #32
+	sdiv r3, r0, r3


Replace these two with asr r3, r0, #5? Shorter and matches m0 implementation

ppannuto · 2017-11-22T03:55:10Z

arch/cortex-m0/src/lib.rs

+    /* NVIC.ICER[r0 / 32] = 1 << (r0 & 31) */
+	movs	r2, #31
+	asrs	r3, r0, #31
+	ands	r3, r2


Now I'm a bit more confused, as it looks like this is and'ing with the shifted r0? I think commenting each step will alleviate confusion here.

alevy · 2017-11-22T18:21:28Z

@ppannuto I cleaned up and commented the assembly for disabling nvics in the ISR.

I think the problem was that I had compiled the assembly from C code that used signed (rather than unsigned) integers so GCC was be overly clever. I re-did it with unsigned ints and the result was good enough for me to understand and comment.

ppannuto

This looks good to me now. Only blocking thing I have left is the spelling of deferred :)

ppannuto · 2017-11-23T01:51:57Z

arch/cortex-m4/src/lib.rs

+
+    /* r0 = 1 << (r0 & 31) */
+	movs r3, #1        /* r3 = 1 */
+	and	r0, r0, #31    /* r0 = r0 & r2 (31) */


nit: The assembly is right, the comment is wrong (r2 not involved here)

ppannuto · 2017-11-23T01:54:09Z

chips/nrf51/src/chip.rs

+                    TIMER1 => nrf5x::timer::ALARM1.handle_interrupt(),
+                    TIMER2 => nrf5x::timer::TIMER2.handle_interrupt(),
+                    UART0 => uart::UART0.handle_interrupt(),
+                    _ => debug!("NvicIdx not supported by Tock\r\n"),


nit: don't need \r\n here, debug! adds the line endings for you

ppannuto · 2017-11-23T02:01:14Z

chips/sam4l/src/flashcalw.rs

    GPFRLO,
 }

+static DEFERED_CALL: DeferedCall = unsafe { DeferedCall::new(Task::Flashcalw) };


This is pervasive -- and don't hate me -- s/defered/deferred

@ppannuto I couldn't hate you if I wanted to.

ppannuto · 2017-11-23T02:05:09Z

chips/sam4l/src/dma.rs


    pub fn handle_interrupt(&mut self) {
        let registers: &mut DMARegisters = unsafe { mem::transmute(self.registers) };
+        registers.interrupt_disable.set(0xffffffff);


nit: you use !0 pretty much everywhere else

alevy · 2017-11-23T17:11:29Z

@ppannuto addressed your comments. I think also all of @bradjc's comments are addressed.

(Fine if approving/merging this waits until after Thanksgiving break)

niklasad1 · 2017-11-25T23:05:10Z

@alevy yeah, I have noticed that the ble_passive_scan app hangs too.

But I haven't understood how to debug user-space applications so I have ignored it but perhaps best to create an issue! I thought it ran out of memory but I don't know!

bradjc requested changes Nov 17, 2017

View reviewed changes

ppannuto requested changes Nov 20, 2017

View reviewed changes

alevy force-pushed the feature/interruptopt branch from 5ab0566 to 5482c8a Compare November 21, 2017 01:05

alevy changed the title ~~Replace SAM4L queue-based interrupts with NVIC regs~~ Replace queue-based interrupts with NVIC regs Nov 21, 2017

alevy force-pushed the feature/interruptopt branch from 4ecfbfc to 6d58a5f Compare November 21, 2017 14:40

bradjc requested changes Nov 21, 2017

View reviewed changes

alevy added 15 commits November 21, 2017 21:49

fix flash with defered call global

c827b7d

Add a more general defered call mechanism to SAM4L

fc426a5

Cleanup unused interrupt_handler macro

45ecd74

Port NRF52 to new cortexm4 interrupt scheme

50a6f16

nit

c9bc26e

Port generic_isr to cortex-m0

eb86491

Remove unneeded no_mangle

7b83b74

Port NRF51 to new interrupt handling scheme

4ee26fe

Address nits

b438514

Generate assembly for NVIC disabling from C

d7ccf5b

rustfmt

146789b

Oops wrong value completely

b59580a

Cortex-M0: need to save r4-r7 in ISR before overwriting them

ae11a94

remove unneeded defered call

b87b353

alevy force-pushed the feature/interruptopt branch from 77ac17f to b87b353 Compare November 22, 2017 02:53

ppannuto requested changes Nov 22, 2017

View reviewed changes

Clean up and comment generic_isr assembly

ce71eae

alevy force-pushed the feature/interruptopt branch from e24c4c5 to ce71eae Compare November 22, 2017 20:56

ppannuto requested changes Nov 23, 2017

View reviewed changes

nits

c460714

alevy mentioned this pull request Nov 27, 2017

Add nRF52 I2C driver #693

Merged

3 tasks

bradjc approved these changes Nov 27, 2017

View reviewed changes

bradjc added the kernel label Nov 27, 2017

ppannuto approved these changes Nov 27, 2017

View reviewed changes

ppannuto merged commit 1e96512 into tock:master Nov 27, 2017

alevy deleted the feature/interruptopt branch November 27, 2017 22:29

niklasad1 mentioned this pull request Nov 29, 2017

[nRF5X] ble_passive_scanning app hangs #698

Closed

bradjc mentioned this pull request Jun 30, 2018

cortex-m4: move hardfault handler to arch #1051

Merged

2 tasks

hudson-ayers mentioned this pull request Jul 17, 2020

Building Tock with stable rustc #1654

Open

29 tasks

Uh oh!

Replace queue-based interrupts with NVIC regs #687

Replace queue-based interrupts with NVIC regs #687

Uh oh!

Conversation

alevy commented Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Overview

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

Uh oh!

bradjc left a comment

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!

bradjc commented Nov 17, 2017

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!

alevy commented Nov 21, 2017

Uh oh!

niklasad1 commented Nov 21, 2017

Uh oh!

alevy commented Nov 21, 2017

Uh oh!

alevy commented Nov 21, 2017

Uh oh!

bradjc left a comment

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!

ppannuto left a comment

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!

alevy commented Nov 22, 2017

Uh oh!

ppannuto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

alevy commented Nov 15, 2017 •

edited

Loading

niklasad1 commented Nov 25, 2017 •

edited

Loading