Page MenuHomeFreeBSD

Allwinner A10 convert to ARM_INTRNG
ClosedPublic

Authored by manu_bidouilliste.com on Mar 7 2016, 6:03 PM.
Tags
Referenced Files
F104205136: D5573.diff
Wed, Dec 4, 5:49 PM
Unknown Object (File)
Mon, Dec 2, 6:26 AM
Unknown Object (File)
Sat, Nov 30, 2:34 AM
Unknown Object (File)
Sat, Nov 30, 12:17 AM
Unknown Object (File)
Sun, Nov 17, 1:46 PM
Unknown Object (File)
Thu, Nov 7, 4:25 AM
Unknown Object (File)
Oct 25 2024, 11:26 PM
Unknown Object (File)
Oct 21 2024, 3:02 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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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 - subversion.
manu_bidouilliste.com added a project: ARM.
jmcneill added inline comments.
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.

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

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?

manu_bidouilliste.com added inline comments.
sys/arm/allwinner/aintc.c
84 ↗(On Diff #14161)

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

manu_bidouilliste.com added inline comments.
sys/arm/allwinner/aintc.c
110 ↗(On Diff #14161)

Not anymore, thanks for pointing that out

287 ↗(On Diff #14161)

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

skra added inline comments.
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.

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)

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()?
EOI should be here now.

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.

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
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.

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

Update since ARM_INTRNG was renamed to INTRNG

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.