Page MenuHomeFreeBSD

riscv: support Allwinner D1 interrupt controllers
Needs ReviewPublic

Authored by julien.cassette_gmail.com on Apr 2 2022, 11:39 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 22, 6:52 AM
Unknown Object (File)
Tue, Apr 16, 2:51 PM
Unknown Object (File)
Nov 14 2023, 8:21 AM
Unknown Object (File)
Nov 12 2023, 8:19 AM
Unknown Object (File)
Nov 11 2023, 11:33 PM
Unknown Object (File)
Nov 11 2023, 3:02 PM
Unknown Object (File)
Nov 10 2023, 9:25 AM
Unknown Object (File)
Nov 8 2023, 9:08 AM

Details

Reviewers
None
Group Reviewers
riscv
Summary

The main interrupt controller of the D1 is the PLIC, but it does not
provide any interface for configuring IRQ trigger type or wakeup.

These features are supported through registers in a separate memory
region (RISCV_CFG), and the device tree defines another interrupt
controller (INTC) as a wrapper around the PLIC and those registers.

Since most peripherals use INTC as their interrupt parent, and do not
use IRQ trigger type or wakeup features, do the following:

  • add the D1 compatible data to the PLIC driver,
  • add a stub driver for INTC to route things back to the PLIC.

Signed-off-by: Julien Cassette <julien.cassette@gmail.com>

Test Plan

A driver can allocate IRQ resources successfully.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 46110
Build 42999: arc lint + arc unit

Event Timeline

Don't see why the PLIC changes are needed, but if they are they should be separated out from the addition of the D1 interrupt controller.

sys/riscv/riscv/plic.c
386

I don't see why this is needed? Anything that references the PLIC should just work with bus_setup_intr etc.

Don't see why the PLIC changes are needed, but if they are they should be separated out from the addition of the D1 interrupt controller.

Hello Jessica.
The first change is because the PLIC is named "thead,c900-plic" in the device tree. The second change is so that INTC can find the PLIC device.
I guessed it made sense to have both changes in the same revision, but I can create an extra revision for only 1 line if you want.
Regards, Julien.

sys/riscv/riscv/plic.c
386

This is so that aw_d1_intc can find its parent device using OF_device_from_xref.

sys/riscv/riscv/plic.c
386

Why does it care? It just needs to register an interrupt via the bus APIs like any other device?

sys/riscv/allwinner/aw_d1_intc.c
160

These don't make any sense. If this allwinner,sun20i-d1-intc device is just a wrapper around the PLIC that forwards every request directly to it, what on earth is the hardware doing and why does it exist?

sys/riscv/allwinner/aw_d1_intc.c
160

It's not really nice or helpful to say that "this does not make any sense" just because you don't understand.
The hardware is clearly described in the device tree and in the D1 user manual: the registers associated to INTC are at 0x0601000 and allow configuration of IRQ wakeup enable bits.
This is similar to aw_r_intc for ARM devices.

sys/riscv/riscv/plic.c
386

It's done the same in aw_r_intc. If you have a suggestion of a modification please do.

sys/riscv/allwinner/aw_d1_intc.c
160

Except you don't do any of that. You don't even map a memory resource (though you do have an unused res field in the softc which I guess is for that). This kind of information should be in the commit message, because as it stands this driver literally does nothing other than act as a transparent wrapper around the PLIC.

Update the commit message, and remove unused 'res'

sys/riscv/allwinner/aw_d1_intc.c
160

Yes, this is only a pass-through to the PLIC at the moment so that we have working interrupts.
I have updated the commit message so that it is clearer and I have removed the unused res.

I guess this commit gives a little more context. So because the device tree declares &intc to be the interrupt parent for several peripherals, we need this stub driver in order to properly route things back to the PLIC, is that right?

sys/riscv/allwinner/aw_d1_intc.c
297

This is on the same pass as the PLIC driver, what if the PLIC doesn't attach first? Maybe BUS_PASS_ORDER_LATE is needed.

sys/riscv/riscv/plic.c
240–242

This could be converted to a ofw_compat_data table instead.

julien.cassette_gmail.com marked an inline comment as not done.

Correct pass level, use ofw_compat_data table

I guess this commit gives a little more context. So because the device tree declares &intc to be the interrupt parent for several peripherals, we need this stub driver in order to properly route things back to the PLIC, is that right?

Yes that's also what I understood from that commit and aw_r_intc.

sys/riscv/allwinner/aw_d1_intc.c
297

Thanks, I saw that it needed 2 passes to attach, but then I forgot. With BUSS_PASS_ORDER_LATE it attaches properly now.

sys/riscv/riscv/plic.c
240–242

Sure, it's done