Page MenuHomeFreeBSD

Allwinner A10 convert to ARM_INTRNG
ClosedPublic

Authored by manu_bidouilliste.com on Mar 7 2016, 6:03 PM.

Details

Summary

This convert the interrupt controller for Allwinner A10 to ARM_INTRNG.
This works with and without ARM_INTRNG but the new default is with ARM_INTRNG

Test Plan

Patch, compile and test on A10 hardware.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3187
Build 3220: arc lint + arc unit

Event Timeline

manu_bidouilliste.com retitled this revision from to Allwinner A10 convert to ARM_INTRNG.
manu_bidouilliste.com updated this object.
manu_bidouilliste.com edited the test plan for this revision. (Show Details)
manu_bidouilliste.com added a reviewer: ARM.
manu_bidouilliste.com set the repository for this revision to rS FreeBSD src repository.
manu_bidouilliste.com added a project: ARM.
jmcneill added inline comments.
sys/arm/allwinner/aintc.c
0

space between if and (irq

0

Avoid function calls in declarations

1

OF_xref_from_node returns phandle_t

3

space between dev, and xref

3

Avoid function calls in declarations

5

These locking macros don't appear to be used

7

Avoid function calls in declarations

16

Avoid function calls in declarations

30

You should evaluate this condition before unlocking the mutex.

54

You should evaluate this condition before unlocking the mutex.

sys/arm/allwinner/aintc.c
1

intr_pic_register wants a intptr_t.

Fix some style(9) issues.

jmcneill added inline comments.Mar 7 2016, 8:33 PM
sys/arm/allwinner/aintc.c
1

Maybe an explicit cast is in order. Also, "function calls in declarations" thing.

Change xref to phandle_t and cast it to intptr_t for intr_pic_register.

manu_bidouilliste.com marked 12 inline comments as done.Mar 7 2016, 9:42 PM

Add myself to copyright.

andrew added inline comments.Mar 26 2016, 11:27 AM
sys/arm/allwinner/aintc.c
0

These should be sorted.

0

Is this used in the intrng case?

2–3

There is no need for this label if we're just returning.

9

What is this protecting?

84

Did you mean to remove this?

manu_bidouilliste.com marked an inline comment as done.Mar 26 2016, 11:35 AM
manu_bidouilliste.com added inline comments.
sys/arm/allwinner/aintc.c
84

Yes, this should never had been there, D5663 add correct support for the NMI controller

manu_bidouilliste.com marked an inline comment as done.Mar 26 2016, 11:46 AM
manu_bidouilliste.com added inline comments.
sys/arm/allwinner/aintc.c
0

Not anymore, thanks for pointing that out

9

Might be a leftover of some old code, I'll check that.

Order includes
Only use a10_aintc_sc variable if not compiled with ARM_INTRNG
Remove unneeded lock
Release resources if fails to attach
Rename aintc.c to a10/a10_intc.c

emaste added a subscriber: emaste.Apr 7 2016, 3:13 PM

Update diff to last INTR changes.

skra added a subscriber: skra.Apr 9 2016, 10:02 AM
skra added inline comments.
sys/arm/allwinner/a10/a10_intc.c
172

Checking the value for 0 here would avoid the following loop and safe time.

213

Extra line.

216

-1 is returned when no pending interrupt is found, IMO, the interruption was spurious then.

317

Typically, it should be "end of interrupt" here only. I don't see corresponding a10_intr_mask() call in interrupt dispatch path. Should be clearing of pending register enough here?

When interrupt is being dispatched, either only pic_post_filter() is called when there is only filter in an interrupt or both pic_pre_ithread() and pic_post_ithread are called when there is (also) a handler. See intr_event_handle() in kern_intr.c.

Split the EOI part of intr_unmask and call it when irq == 0 (NMI)
Print that we have a spurious interrupt if no interrupt is pending.
Fix style(9)

manu_bidouilliste.com marked 4 inline comments as done.Apr 9 2016, 11:53 AM
skra added a comment.Apr 9 2016, 4:18 PM

I was so curious about these enable/disable and mask/unmask registers that I've downloaded manual and looked at Linux implementation for unpublished "features". So, after this will be accepted and committed, there may be done some improvements like it looks that the interrupts could be unmasked permanently and only enable/disable registers could be used. Further, currently active interrupt on CPU irq input can be found in SW_INT_VECTOR_REG; the read zero must be checked in pending register 0, bit 0 to be sure if it's spurious irq or not.

sys/arm/allwinner/a10/a10_intc.c
232

IMO, a10_pending_irq(sc) returns "int", it's printed as %d, so I would join these ifs, and always print irq.

235

EOI is not in a10_intr_mask() now, so it should be called here.

312

Is it same like a10_intr_disable_intr()?
EOI should be here now.

329

Just suggestion. Move the check inside a10_intr_eoi(). You can make the function inlined. But, it's really only up to you.

sys/arm/allwinner/a10/a10_intc.c
232

I didn't wanted to print "-1", that's why I've split the printfs'

235

True, I'll add that.

312

Same as above, I'll add that.

329

I wanted to avoid function calls but making it inlined would do the job, I'll change that.

skra added a comment.Apr 9 2016, 6:08 PM
In D5573#125735, @skra wrote:

I was so curious about these enable/disable and mask/unmask registers that I've downloaded manual and looked at Linux implementation for unpublished "features". So, after this will be accepted and committed, there may be done some improvements like it looks that the interrupts could be unmasked permanently and only enable/disable registers could be used. Further, currently active interrupt on CPU irq input can be found in SW_INT_VECTOR_REG; the read zero must be checked in pending register 0, bit 0 to be sure if it's spurious irq or not.

Well, to be clear. This was a general note only and you are supposed to do nothing WRT this review as it's about converting this controller for ARM_INTRNG.

However, for the future maybe, I have looked at A10 manual as I was wondering how these SW_INT_ENABLE_REG(0-2) and SW_INT_MASK_REG(0-2) should be used. And to know if all registers are RW, so spin mutex must be used. As a side effect I've learned that SW_INT_IRQ_PENDING_REG(0-2) are READ ONLY registers. Of course, only according to manual I have. Maybe there is another which I have not. But you know, a10_intr_eoi() writes to SW_INT_IRQ_PENDING_REG0. So I have looked at Linux implementation and even there irq 0 is acknowledged by writing to SW_INT_IRQ_PENDING_REG0. So my note about unpublished features came from it. It used to be true that Linux knows better than manual.

Anyhow, the conclusion from my reading of manual and Linux implementation of this controller is the following:

  • all interrupts may be disabled and unmasked during device attach. Then, only SW_INT_ENABLE_REG(0-2) may be used in a10_intr_unmask() and a10_intr_mask().
  • active interrupt may be read from SW_INT_VECTOR_REG. It's on bit 31:2. However, when a zero is there, it may be either irq0 or spurious interrupt. So, SW_INT_IRQ_PENDING_REG0 must be read to decide.
sys/arm/allwinner/a10/a10_intc.c
223

In not ARM_INTRNG case, the arm_get_next_irq() is called repeatedly until -1 is returned. So, to behave same, a10_pending_irq() should be called here repeatedly. And it's spurious interrupt only when no pending interrupt is found at the beginning.

Make a10_intr_eoi inline.
In a10_intr loop until irq == -1 to dispatch all irq

manu_bidouilliste.com marked 9 inline comments as done.Apr 10 2016, 12:00 PM

Update since ARM_INTRNG was renamed to INTRNG

skra accepted this revision.Apr 24 2016, 8:57 PM
skra added a reviewer: skra.
This revision is now accepted and ready to land.Apr 24 2016, 8:57 PM
This revision was automatically updated to reflect the committed changes.