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
Details
Details
- Reviewers
skra - Group Reviewers
ARM - Commits
- rS298625: Convert A10 interrupt controller to INTRNG
Patch, compile and test on A10 hardware.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| sys/arm/allwinner/aintc.c | ||
|---|---|---|
| 116 ↗ | (On Diff #14136) | These locking macros don't appear to be used |
| 141 ↗ | (On Diff #14136) | OF_xref_from_node returns phandle_t |
| 178 ↗ | (On Diff #14136) | space between dev, and xref |
| 216 ↗ | (On Diff #14136) | space between if and (irq |
| 315 ↗ | (On Diff #14136) | Avoid function calls in declarations |
| 324 ↗ | (On Diff #14136) | Avoid function calls in declarations |
| 334 ↗ | (On Diff #14136) | Avoid function calls in declarations |
| 343 ↗ | (On Diff #14136) | Avoid function calls in declarations |
| 357 ↗ | (On Diff #14136) | You should evaluate this condition before unlocking the mutex. |
| 381 ↗ | (On Diff #14136) | You should evaluate this condition before unlocking the mutex. |
| sys/arm/allwinner/aintc.c | ||
|---|---|---|
| 141 ↗ | (On Diff #14136) | intr_pic_register wants a intptr_t. |
| sys/arm/allwinner/aintc.c | ||
|---|---|---|
| 139 ↗ | (On Diff #14140) | Maybe an explicit cast is in order. Also, "function calls in declarations" thing. |
| sys/arm/allwinner/aintc.c | ||
|---|---|---|
| 35–44 ↗ | (On Diff #14161) | These should be sorted. |
| 84 ↗ | (On Diff #14161) | Did you mean to remove this? |
| 110 ↗ | (On Diff #14161) | Is this used in the intrng case? |
| 195–196 ↗ | (On Diff #14161) | There is no need for this label if we're just returning. |
| 287 ↗ | (On Diff #14161) | What is this protecting? |
Comment Actions
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
| sys/arm/allwinner/a10/a10_intc.c | ||
|---|---|---|
| 172 ↗ | (On Diff #15028) | Checking the value for 0 here would avoid the following loop and safe time. |
| 213 ↗ | (On Diff #15028) | Extra line. |
| 216 ↗ | (On Diff #15028) | -1 is returned when no pending interrupt is found, IMO, the interruption was spurious then. |
| 317 ↗ | (On Diff #15028) | 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. |
Comment Actions
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)
Comment Actions
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 | ||
|---|---|---|
| 231 ↗ | (On Diff #15036) | IMO, a10_pending_irq(sc) returns "int", it's printed as %d, so I would join these ifs, and always print irq. |
| 234 ↗ | (On Diff #15036) | EOI is not in a10_intr_mask() now, so it should be called here. |
| 311 ↗ | (On Diff #15036) | Is it same like a10_intr_disable_intr()? |
| 328 ↗ | (On Diff #15036) | 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 | ||
|---|---|---|
| 231 ↗ | (On Diff #15036) | I didn't wanted to print "-1", that's why I've split the printfs' |
| 234 ↗ | (On Diff #15036) | True, I'll add that. |
| 311 ↗ | (On Diff #15036) | Same as above, I'll add that. |
| 328 ↗ | (On Diff #15036) | I wanted to avoid function calls but making it inlined would do the job, I'll change that. |
Comment Actions
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 | ||
|---|---|---|
| 222 ↗ | (On Diff #15036) | 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. |