Page MenuHomeFreeBSD

INTRNG generalization step 1 - new PIC interface and BUS related functions
ClosedPublic

Authored by skra on Mar 24 2016, 7:59 PM.

Details

Summary

Originally, INTRNG has been designed for FDT like systems only. And while the current implementation was done more general, it turned out that it's not enough.

Thus, INTRNG must be reworked to answer needs of not FDT systems, on demand interrupt setups (e.g., not described in FDT dtb), MSI and MSI-X interrupts and others.

It's not sure now how some delicate things will be solved, however, the main result of this step should be a stable PIC interface which allows to start reworking of other PICs for use with INTRNG without dread of next steps.


GENERALLY

(A) INTRNG works only with global interrupt numbers. Each of them is associated with some interrupt source (ISRC) on some interrupt controller (PIC). And only a PIC can know which global interrupt numbers are associated with its ISRCs. So, if someone wants to know a global interrupt number of some specific interrupt, the query must always end in some PIC.

Basically, a query should be done thru INTRNG framework. However, in some special cases when a PIC is an integral part of another device, it could be done within that device (not thru INTRNG framework). For example, someone could ask a GPIO framework for an interrupt for pin 20 on gpio1. If a PIC is a part of that gpio1 device, the query may be resolved internally.

(B) On modern SOCs, a description of an interrupt may be quite complex and may differ from PIC to PIC. It identifies an interrupt itself as well as its configuration. A description is not only associated with an interrupt, but also and mainly with a device. An interrupt may be utilized by more devices either at the same time or separately on user demand.

Thus, an interrupt may be configured repeatedly according to a device which just uses it at the moment. Therefore an interrupt description for a device must be stored somewhere to be found when needed. There is an idea to use struct resource. The r_virtual field could store a pointer to it. It's the simplest (perfect) solution as struct resource is already owned by a device and passed as an argument to interrupt related bus methods.


IDEA

A struct intr_map_data is defined to represent various interrupt descriptions. The struct definition should be opaque for INTRNG framework for two reasons. First, only associated PIC could know the semantics of an interrupt description. Second, only creator (e.g. FDT bus driver) and recepient (e.g. FDT knowing PIC) of this struct must know its definition.

To get a global interrupt number thru INTRNG framework, one general mapping function and correspondig PIC method are implemented:

intr_map_irq() => pic_map_intr()

which take PIC identification and struct intr_map_data as arguments.

For general use, four INTRNG functions and corresponding PIC methods are implemented to be used for BUS resource methods implementation.

bus_alloc_resource(): intr_alloc_irq() => pic_alloc_intr()
bus_setup_intr(): intr_setup_irq() => pic_setup_intr()
bus_teardown_intr(): intr_teardown_irq => pic_teardown_intr()
bus_release_intr(): intr_release_irq() => pic_release_intr()

A struct resource and struct intr_map_data are passed to these functions and methods as arguments to be general enough. In these structs, data needed for PIC to configure an interrupt may be stored. A struct intr_map_data argument may be NULL, meaning that there is no configuration.

A bus_config_intr() method is not standard as struct resource is not passed as an argument to it. It's not supported by INTRNG framework. An interrupt configuration should be passed to PIC thru struct intr_map_data.

Only global interrupt numbers must be held in struct resource which is passed as an argument to INTRNG functions!


IN THIS STEP

The main result of this step should be a stable PIC interface.

(1) The idea above is implemented.

(2) ISRC allocations was moved from INTRNG framework to PICs. They are allocated at the beginning and global interrupt numbers are assigned to them at the same time. There is no more a need for calling a mapping function to start using any global interrupt number (the number still must be correct).

(3) Temporarily, struct intr_map_data is stored in a table in INTRNG framework to not hold back reworking of other PICs before real solution will be implemeted. Such solution is not expected to change PIC interface.

However, this temporary solution creates two global interrupt number spaces at this moment. The first space associated with ISRCs is the real one. The second space associated with table keeping struct intr_map_data for devices is artificial one. Each interrupt number from second space duplicates some number from first space. An interrupt number from first space can be duplicated even multiple times in second space.

Two (ACPI and FDT) types for struct intr_map_data are defined for now and two corresponding map functions are implemented.

intr_acpi_map_irq()
intr_fdt_map_irq()

Almost nothing is changed with regard to previous behaviour of INTRNG framework. However, as these functions still may be called before corresponding PIC is attached, a situation is better now as the issue that two ISRCs are allocated for same interrupt cannot happen. The issue that two global interrupt numbers are associated with same ISRCs will be resolved in next step.

(4) A struct intr_irqsrc is cleaned up now as fdt data is no more a part of it.

(5) A struct resource is also passed to the following INTRNG framework functions to keep line with other ones:

intr_bind_irq()
intr_describe_irq()

(6) Some other PIC methods are renamed to reflect new naming and a demand done before.


STABLE PIC interface

Each PIC should define own ISRC struct with struct intr_irqsrc on top of it, allocate all its ISRCs and register them to INTRNG by calling intr_isrc_register().

Minimal PIC implementation must provide the following methods:

pic_disable_intr() ... called to disable an interrupt when there is no handler for it
pic_enable_intr() ... called to enable an interrupt when first handler is added for it
pic_post_filter() ... for MI interrupt framework
pic_post_ithread() ... for MI interrupt framework
pic_pre_ithread() ... for MI interrupt framework

If a PIC supports some interrupt mapping method(s), it must provide

pic_map_intr() ... to map data in struct intr_map_data into global interrupt number

If a PIC needs to setup (or teardown) an interrupt, it must provide

pic_setup_intr() ... to setup an interrupt
pic_teardown_intr() ... to teardown an interrupt

Both methods are called with every handler added for an interrupt, so a PIC could check that configurations associated with handlers do match.

There are two more methods, however, not used now

pic_alloc_intr() ... called to be noticed that someone allocate a resource for an interrupt
pic_release_intr() ... called to be noticed that someone release a resource for an interrupt

Finally, there are four methods for SMP case

pic_bind_intr() ... to bind an interrupt to some CPU
pic_init_secondary() ... to init PIC on AP
pic_ipi_send() ... to send an IPI
pic_ipi_setup() ... to setup an IPI

Only pic_post_filter() and pic_pre_ithread() are called in an interrupt context. They could have a local scope. I.e., their actions should or may be limited to current cpu only. However, as a matter of fact of pic_pre_ithread() local scope, pic_post_ithread() is called from an ithread context which may run on another cpu.

Test Plan

Tested on pandaboard and tegra.
It would be nice if somebody could test it on

  • IMX6,
  • ARMADAXP with ARM_INTRNG set.

There is no MIPS kernel configuration with MIPS_INTRNG set in tree at this moment

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

skra updated this revision to Diff 14583.Mar 24 2016, 7:59 PM
skra retitled this revision from to INTRNG generalization step 1 - new PIC interface and BUS related functions.
skra updated this object.
skra edited the test plan for this revision. (Show Details)
skra added reviewers: andrew, ian, adrian, kan.
skra set the repository for this revision to rS FreeBSD src repository.
skra added projects: ARM, MIPS.
skra added subscribers: ARM, MIPS.
skra updated this revision to Diff 14599.Mar 25 2016, 3:15 PM
skra updated this object.
skra edited the test plan for this revision. (Show Details)

Remove unused ipi_next_num variable.
Include opt_acpi.h to subr_intr.c.
Remove FDT ifdefs from mpic.c as the driver is only for FDT.

The pic_map_intr() method had to set a global interrupt number to output argument. It was confusing so ISRC pointer is now set to output argument. Really, it was my fault that all reworked PICs was setting a local interrupt number. It has worked on panda and tegra as they have only root PICs reworked and local to global interrupt mapping was 1: 1. It's fixed now.

skra updated this revision to Diff 14673.Mar 28 2016, 10:51 AM
skra updated this object.

Change intr_isrc_dispatch() return value from void to int to signal that an interrupt (ISRC) was not handled. Even ISRC with handlers does not guarantee that it happens. So, it's only a PIC responsibility now to act with a stray interrupt - e.g., disable interrupt and does EOI. Note that calling of PIC_DISABLE_INTR() from INTRNG was not enough.

emaste added a subscriber: emaste.Mar 28 2016, 4:43 PM
skra updated this revision to Diff 14737.Mar 30 2016, 2:39 PM
skra updated this object.

(1) An ipi argument was added to PIC_IPI_SEND method to explicitly specify which ipi should be send. Some controllers use one ISRC for all IPIs (e.g. RPI2 uses a mailbox interrupt), so this simplifies things for them.
(2) Update to current was done.

andrew edited edge metadata.Mar 31 2016, 10:04 AM

This feels a little over engineered.

For MSI we will need to allocate space within the global interrupt space. We will then tell the hardware what interrupts to use. This means the map function is just to tell the PCI hardware how to send interrupts to the CPU. From the point of view of intrng it will look like a child is allocating space in the parents irq space.

By my reading of the ACPI code it already handles the delayed configuration by handling it in acpi_alloc_resource so there should be no need to store it's details a second time.

sys/arm/arm/gic.c
392 ↗(On Diff #14737)

Do you need to zero the memory if you're just going to write to it later withing the fuction?

730 ↗(On Diff #14737)

Why not the following?

switch (ncells) {
case 1:
   ...
   return (0);
...
}
801–807 ↗(On Diff #14737)

When ACPI is supported I expect there will be a second attachment created. It will mean this function will only ever handle FDT.

sys/arm/arm/nexus.c
284 ↗(On Diff #14583)

The acpi code calls BUS_CONFIG_INTR.

sys/kern/subr_intr.c
536 ↗(On Diff #14737)

This assumes a 32 bit u_int

skra added a comment.Mar 31 2016, 4:08 PM

This feels a little over engineered.

I'm not sure what exactly is, by your words, over engineered. In fact, intrng is quite more slim and simple now. The world of modern SOCs is complex, and intrng is a framework which should be used by many. Thus there is no place to make it less general. Maybe, the summary I wrote for this change could look complicated. But the result - this change - make it quite simple.

For MSI we will need to allocate space within the global interrupt space. We will then tell the hardware what interrupts to use. This means the map function is just to tell the PCI hardware how to send interrupts to the CPU. From the point of view of intrng it will look like a child is allocating space in the parents irq space.

Well, I'm not sure what did you want to say by this. MSI can be simple. MSI is not only user of intrng. MSI-able PIC may be on another sub-tree than a MSI client. There is no specific MSI code in intrng now. I can imagine that MSI-able PICs could register themselves to intrng with some MSI flag. And anybody who will want a MSI interrupt will query intrng for it, and the query will be passed to right PIC. And such query will be done thru intr_map_irq().

By my reading of the ACPI code it already handles the delayed configuration by handling it in acpi_alloc_resource so there should be no need to store it's details a second time.

Well, ACPI is not the only world. An interrupt number is already saved in struct resource. Our idea is to save its configuration there too if needed. Note that intrng is a framework which should make things possible and easy for many, not just for ACPI.

sys/arm/arm/gic.c
392 ↗(On Diff #14737)

This function is called exactly one time. So M_ZERO is not a big deal. In fact, ISRCs and PIC registering and deregistering is a piece of code to be think over more. I'm trying to think up (to establish) some procedure how to do it and then, I'm prepared to apply it to all PICs. And, it deserves separate commit.

730 ↗(On Diff #14737)

I try to not use a switch if there are only two cases. Especially if the case when ncells == 1 shouldn't be there at all. Further, this function should be removed in next step and I have tried to not change things not related to this change. This file was changed only to work after changes done in intrng.

801–807 ↗(On Diff #14737)

AFAIK, this file is not used in ACPI like systems. If it will be then the right code must be added here. In fact, I was about to remove the ACPI parts from intrng several times as it's not used now and maybe incorrect. But it's still there to show anybody how it could be used for other cases then FDT one.

sys/arm/arm/nexus.c
284 ↗(On Diff #14583)

Yes, I know. But if I'm not wrong, BUS_CONFIG_INTR is not called from "client" driver. It's called from some bus driver on a way up to nexus. So if such bus driver will use intrng, there is intrng-like-way how to config an interrupt. And the fact that struct resource is not an argument of this call shows, IMO, clearly how not standard (hackish) the call is.

sys/kern/subr_intr.c
536 ↗(On Diff #14737)

Yes, u_int is now 32 bit in all fbsd supported platforms. But I see your point and it's correct. However, this function will be removed in next step.

skra added inline comments.Mar 31 2016, 9:50 PM
sys/arm/arm/gic.c
730 ↗(On Diff #14737)

Hmm, the note about removing of this function somehow sneaked in. Likely, I was thinking about another comment in parallel and my thoughts mixed up. ;)

This revision was automatically updated to reflect the committed changes.