Page MenuHomeFreeBSD

Add interrupt controller interface on ARM & allow to use stacked ICs
AbandonedPublic

Authored by ian on Aug 11 2014, 4:56 PM.

Details

Reviewers
andrew
imp
jceel
Summary
Introduces hierarchical interrupt controllers in the system, allowing
to easily utilize multiplexers such as GPIOs to serve as interrupt
sources. nexus driver is a top-level interrupt controller, exposing
single interruprt (IRQ) on ARM. The GIC or any other used interrupt
controller setups handler on that interrupt, exposing new IRQs
available for other peripherals. In an example SoC, interrupt hierarchy
may look like that:

nexus0 (1 interrupts)
|
\-- gic0 (160 interrupts, uses irq nexus0:0)
    |
    \-- gpio0 (8 interrupts, uses irq gic0:42)
    |   |
    |   \-- mmcsd0 (uses irqs gpio0:1, gpio0:2)
    |   \-- spi0 (uses irq gpio0:3)
    |   ...
    \-- gpio1 (8 interrupts, uses irq gic0:43)
    \-- ehci0 (uses irq gic0:109)
    ...

That change should not break any existing ports in any way, except
for need to add 'arm/arm/intr.c' to 'files.*' of existing ports,
 as it's no longer compiled-in by default.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

jceel updated this revision to Diff 1046.Aug 11 2014, 4:56 PM
jceel retitled this revision from to Add interrupt controller interface on ARM & allow to use stacked ICs.
jceel updated this object.
jceel edited the test plan for this revision. (Show Details)
jceel added reviewers: imp, ian, andrew.
jceel updated this object.
andrew edited edge metadata.Aug 11 2014, 9:59 PM

A few points I found from a quick look through.

sys/arm/arm/gic.c
171

This comment has sneaked in.

sys/arm/arm/mp_machdep.c
362

This comment doesn't make sense, I see no 0xdeadbeef here.

sys/arm/broadcom/bcm2835/files.bcm2835
24

Do we need this as it's in the global files.arm?

sys/boot/fdt/dts/arm/pandaboard.dts
61

This looks odd, does the GIC use irq 0? My copy of the Linux dts doesn't have this so we shouldn't require it.

sys/conf/files.arm
29

Shouldn't we still allow the old interrupt code here until we enable intrng on all SoCs?

jceel added inline comments.Aug 12 2014, 8:06 AM
sys/arm/arm/gic.c
173–174

That's clearly wrong. I've missed these two lines when doing merge.

sys/arm/arm/mp_machdep.c
362

Yes, that comment seems invalid. It is also present in head.

sys/arm/broadcom/bcm2835/files.bcm2835
24

Nope, this will be removed.

sys/boot/fdt/dts/arm/pandaboard.dts
61

It's a temporary solution. We were talking with Ian how to handle that and the best solution will be to allow ICs to setup interrupt 0 without having such resource assigned (ignore error from bus_alloc_resource*() and proceed to bus_setup_intr()).

ian added inline comments.Aug 14 2014, 3:07 AM
sys/arm/arm/gic.c
277

the changes in this routine break the spurious interrupt detection, which counts on last_irq being passed in from what used to be a loop in the caller, and it also requires the old standard of returning -1 to mean "no more active irqs".

we'll need to think about how to report spurious interrupts with intrng. an occasional spurious interrupt is no big deal, but lots of them mean trouble in a driver (or, recently, trouble in our whole armv6 architecture really, which I hacked around with the arm_irq_memory_barrier() stuff).

280

this isn't right, the EOI can't be done until after the filter handler is done. recent changes on -current moved this to a post_filter function.

sys/arm/arm/intr.c
100

this static buffer is not SMP-safe (although I think it would take something like two hot-plug devices being handled concurrently on two different cores both of which trigger the loading of drivers and resource allocation, and both arriving here at the same time to get a glich).

sys/arm/arm/intrng.c
107

arm_intrnames_init() can go, it's no longer referenced from anywhere except intr.c

199

another place where static buffers may not be SMP-safe

249

In my fdt_clock work I store a similar table of dev<->node references, and Nathan said, well, I'm not sure I have a complete handle on what he was saying, but I'll paste it here so we can remember to follow up...

<DamnHippi> nathanw: so you're saying fdt_clock_register_provider() should call ofw_bus_get_node() then pass that value through OF_xref_phandle() before storing it?
<DamnHippi> I thought OF_xref_phandle() only worked on &foo type references within the data.
<nathanw> it does
<nathanw> there is not an inverse operation at the moment
<nathanw> there should be
<nathanw> it would be easy to do
<nathanw> I'm happy to make an API for you

Also, if we're going to have multiple places storing dev<->phandle lookup lists/tables, maybe we should put some common code for it into dev/fdt or dev/ofw. gpio and pinmux stuff also tends to have lots of &node refs in properties that we may some day handle by making the node reference correspond to a device_t.

372

if all masking/unmasking/eoi now comes through these routines, then maybe this would be the new place where arm_irq_memory_barrier() calls should go, instead of individual IC drivers.

sys/arm/arm/nexus.c
281

I think this might be the place to check for res == NULL and use that to set up the root IC. We can validate that flags includes INTR_CONTROLLER, and perhaps also that no root controller is regigistered already.

418

I'm curious why this cpu_to_fdt32() is needed, when the resource loops in simplebus and ofwbus that call this code have already done OF_getencprop() which includes endian-swapping already.

431

some ICs put the irq number in intr[1]. this is that whole ugly "only the IC driver knows how to decode the interrupts=<...> cell contents" thing.

sys/arm/include/fdt.h
50

the parens on these macros aren't quite right, should be

#define FDT_MAP_IRQ(node, pin) arm_fdt_map_irq((node), (pin))
​#define FDT_DESCRIBE_IRQ(irq) arm_describe_irq((irq))

likewise the one in the #else below

sys/arm/include/intr.h
42

I have no idea why arm has a psl.h, but I'm pretty sure it isn't needed in here.

43

If this is for trapframe, we can get away with a simple fwd decl of it.

123

arm_intrnames_init() can go away. it used to be referenced from several files, but now it's only called by a SYSINIT local to the same file.

sys/conf/files.arm
29

With Warner's recent config(8) changes we can do that with a line like:

arm/arm/intr.c optional !arm_intrng

sys/conf/options.arm
6

btw, is this an aid to converting platforms one at a time and eventually it will go away, or will we always have both old and new intr code around?

ian commandeered this revision.Oct 22 2015, 6:19 PM
ian edited reviewers, added: jceel; removed: ian.
ian abandoned this revision.Oct 22 2015, 6:20 PM

INTRNG was committed (in somewhat different form) as r289529