Add support for adding a child interrupt controller

[andrew]

Summary

This is a controller that has a common set of IRQs, but will be managed
by a different driver as is the case with the GICv3. The base driver will
handle the interrupt, find it is within the ITS range, then call
intr_child_irq_handler to pass the interrupt to the ITS driver. The ITS
driver will finally handle the interrupt by translating this irq to
a intr_irqsrc and calling intr_isrc_dispatch.

[andrew]

D6437 contains my work in progress ITS driver to show how this change will be used.

[andrew]

Any comments?

[skra]

(1) Well, chained PIC is bound to its parent only by one irq. Considering your change, is there any other difference here except that child PIC is bound to a range of irqs? For example, could it be sufficient to register ISRC with child device_t but keep it in parent PIC?

(2) IMO, most platforms won't need this functionality. So, maybe it could be held under some option? And, maybe there is no need to put this to framework at this moment. IMO, the same thing may be implemented in parent PIC driver as well. Are you expecting that more drivers will use this feature in near future?

[andrew]

In D6436#138609, @skra wrote:

(1) Well, chained PIC is bound to its parent only by one irq. Considering your change, is there any other difference here except that child PIC is bound to a range of irqs? For example, could it be sufficient to register ISRC with child device_t but keep it in parent PIC?

That would need the parent to be able to handle registering children as they may not be a bus child. It would potentially mean we would need to copy the code to handle this to all drivers that allow child interrupt controllers.

(2) IMO, most platforms won't need this functionality. So, maybe it could be held under some option? And, maybe there is no need to put this to framework at this moment. IMO, the same thing may be implemented in parent PIC driver as well. Are you expecting that more drivers will use this feature in near future?

when it's not needed it only increases the struct pic by a little, as we don't expect to see many pics in a system this extra size is minimal. I could also imagine the gicv3m driver could be reworked to use this as it currently knows too much about the internals of the gicv2 driver.

[skra]

In D6436#139055, @andrew wrote:

In D6436#138609, @skra wrote:

(1) Well, chained PIC is bound to its parent only by one irq. Considering your change, is there any other difference here except that child PIC is bound to a range of irqs? For example, could it be sufficient to register ISRC with child device_t but keep it in parent PIC?

That would need the parent to be able to handle registering children as they may not be a bus child. It would potentially mean we would need to copy the code to handle this to all drivers that allow child interrupt controllers.

Hmm, the thing which bothers me really is that I'm not able to define a difference between chained PIC and child PIC. A chained PIC must call bus_setup_intr() to connect itself to its parent. And there is a flag INTR_SOLO which enables calling of a PIC filter function directly, i.e. without a wrap of intr_event_handle(). Your child PIC does not call bus_setup_intr() and call a PIC filter function directly. But it does that on a range of irqs. Does it mean that we should register a chained PIC the same way how a child PIC is? In other words, if a child PIC is registered this way, why not register a chained PIC the same way?

So, what makes a child PIC different? In a chained PIC case, one parent irq is mapped to N irqs of chained PIC. In a child PIC case, N parent irqs are mapped to N irqs of child PIC. A chained PIC may not be a bus child too. Don't we rather need bus_setup_intr() to be effective for a range of irqs?

(2) IMO, most platforms won't need this functionality. So, maybe it could be held under some option? And, maybe there is no need to put this to framework at this moment. IMO, the same thing may be implemented in parent PIC driver as well. Are you expecting that more drivers will use this feature in near future?

when it's not needed it only increases the struct pic by a little, as we don't expect to see many pics in a system this extra size is minimal. I could also imagine the gicv3m driver could be reworked to use this as it currently knows too much about the internals of the gicv2 driver.

I care of the size of code more than the size of struct intr_pic in this case. And, I would like to keep the framework simple if possible.

[andrew]

In D6436#139713, @skra wrote:

Hmm, the thing which bothers me really is that I'm not able to define a difference between chained PIC and child PIC. A chained PIC must call bus_setup_intr() to connect itself to its parent. And there is a flag INTR_SOLO which enables calling of a PIC filter function directly, i.e. without a wrap of intr_event_handle(). Your child PIC does not call bus_setup_intr() and call a PIC filter function directly. But it does that on a range of irqs. Does it mean that we should register a chained PIC the same way how a child PIC is? In other words, if a child PIC is registered this way, why not register a chained PIC the same way?

So, what makes a child PIC different? In a chained PIC case, one parent irq is mapped to N irqs of chained PIC. In a child PIC case, N parent irqs are mapped to N irqs of child PIC. A chained PIC may not be a bus child too. Don't we rather need bus_setup_intr() to be effective for a range of irqs?

A child PIC will manage the allocated irq space, including allocating it's own strict isrc for each interrupt, removing the need from the parent. This means all the parent needs to know is when to pass an interrupt to a child.

INTR_SOLO is also not available as it's definition is surrounded by #ifdef notyet indicating it's not ready.

I care of the size of code more than the size of struct intr_pic in this case. And, I would like to keep the framework simple if possible.

I don't see the amount of new code as a concern, and feel this keeps the framework sufficiently simple.

[skra]

In D6436#140467, @andrew wrote:

In D6436#139713, @skra wrote:

Hmm, the thing which bothers me really is that I'm not able to define a difference between chained PIC and child PIC. A chained PIC must call bus_setup_intr() to connect itself to its parent. And there is a flag INTR_SOLO which enables calling of a PIC filter function directly, i.e. without a wrap of intr_event_handle(). Your child PIC does not call bus_setup_intr() and call a PIC filter function directly. But it does that on a range of irqs. Does it mean that we should register a chained PIC the same way how a child PIC is? In other words, if a child PIC is registered this way, why not register a chained PIC the same way?

So, what makes a child PIC different? In a chained PIC case, one parent irq is mapped to N irqs of chained PIC. In a child PIC case, N parent irqs are mapped to N irqs of child PIC. A chained PIC may not be a bus child too. Don't we rather need bus_setup_intr() to be effective for a range of irqs?

A child PIC will manage the allocated irq space, including allocating it's own strict isrc for each interrupt, removing the need from the parent. This means all the parent needs to know is when to pass an interrupt to a child.

And chained PIC does exactly the same. When a range will be a single number, there is no difference. But don't take me wrong. I just want to define when a PIC should used bus_setup_intr() to connect itself to its parent and when intr_pic_add_handler(). There was a debate about how a chained PIC should be setup to its parent when INTRNG was being created. And bus_setup_intr() was chosen for good reasons. Unfortunately, unexpected result of that decision is that INTR_SOLO was put away by Ian for now because of solo filter prototype does not match filter argument passed to bus_setup_intr().

INTR_SOLO is also not available as it's definition is surrounded by #ifdef notyet indicating it's not ready.

I care of the size of code more than the size of struct intr_pic in this case. And, I would like to keep the framework simple if possible.

I don't see the amount of new code as a concern, and feel this keeps the framework sufficiently simple.

You may be right that the amount of new code is no concern. But, IMO, this thinking leads to the situation that FreeBSD is becoming unusable on small embedded platforms. The kernel is big and there is too much overhead.

And I still have a feeling that this stuff should not be tied to the framework. Both intr_pic_add_handler() and intr_child_irq_handler() need to know nothing about struct pic. The list of child PICs (and the mutex) may be kept in some other structure, allocated by parent PIC and kept in its softc (device control block).

[andrew]

In D6436#140920, @skra wrote:

And chained PIC does exactly the same. When a range will be a single number, there is no difference. But don't take me wrong. I just want to define when a PIC should used bus_setup_intr() to connect itself to its parent and when intr_pic_add_handler(). There was a debate about how a chained PIC should be setup to its parent when INTRNG was being created. And bus_setup_intr() was chosen for good reasons. Unfortunately, unexpected result of that decision is that INTR_SOLO was put away by Ian for now because of solo filter prototype does not match filter argument passed to bus_setup_intr().

So if we call bus_setup_intr the parent will allocate the intr_irqsrc, the child will then need to allocate a second intr_irqsrc for the same interrupt. This will work for a small number of interrupts, but not for say 500. The alternative is there could be some interface for the parent to pass the creation to the child, however this would defeat the point of calling bus_setup_intr

And I still have a feeling that this stuff should not be tied to the framework. Both intr_pic_add_handler() and intr_child_irq_handler() need to know nothing about struct pic. The list of child PICs (and the mutex) may be kept in some other structure, allocated by parent PIC and kept in its softc (device control block).

I don't see how this would not be part of the framework. I'm trying to keep the interdependence between the parent and child at a minimum. This will allow the parent to be built without the child keeping the code size small when a custom kernel config is needed.

[skra]

In D6436#141112, @andrew wrote:

So if we call bus_setup_intr the parent will allocate the intr_irqsrc, the child will then need to allocate a second intr_irqsrc for the same interrupt. This will work for a small number of interrupts, but not for say 500. The alternative is there could be some interface for the parent to pass the creation to the child, however this would defeat the point of calling bus_setup_intr

Still, the question is what makes some PICs privileged to use this new interface. How to justify that some PICs must call bus_setup_intr() and the privileged ones not. This is unclear for me. Hmm, maybe we can say that a child PIC has no own interrupts. And we could call this kind of PICs by more explanatory name?

I don't see how this would not be part of the framework. I'm trying to keep the interdependence between the parent and child at a minimum. This will allow the parent to be built without the child keeping the code size small when a custom kernel config is needed.

Well, you have implemented some support for one kind of PICs. However, there could be more kinds of PICs and each of them may have own support. If we add some entries to struct pic for each of them, it will be very unfortunate. And this is what concerns me.

What about to add new method to pic_if.m to be called by child PIC and use intr_pic_add_handler() as generic implemetation. Define this structure

struct foo {
        struct mtx                 foo_lock;
        SLIST_HEAD(, intr_pic_child) foo_children;
};

to be used by parent PIC device and pass it to intr_child_irq_handler() as argument. Then it will work too and will really be independent. It can be in subr_intr.c as a support for some kind of PICs. And even if it's used by some PICs, it's not tied to the framework.

BTW, I have already discussed with mmel that there could be some support functions for simple PICs as their code is already very similar.

[andrew]

In D6436#141219, @skra wrote:

In D6436#141112, @andrew wrote:

So if we call bus_setup_intr the parent will allocate the intr_irqsrc, the child will then need to allocate a second intr_irqsrc for the same interrupt. This will work for a small number of interrupts, but not for say 500. The alternative is there could be some interface for the parent to pass the creation to the child, however this would defeat the point of calling bus_setup_intr

Still, the question is what makes some PICs privileged to use this new interface. How to justify that some PICs must call bus_setup_intr() and the privileged ones not. This is unclear for me. Hmm, maybe we can say that a child PIC has no own interrupts. And we could call this kind of PICs by more explanatory name?

I would expect any PIC that maps a contiguous range N:N (N > 1) to another PIC could use this interface. To begin with there is only one place this will be used with the GICv3 drivers, however as I mentioned earlier the GICv2/GICv2m drivers could also use this as they fit this pattern. While bus_setup_intr may be able to handle this case it is not very good at it due to the need to allocate multiple intr_irqsrc objects.

I don't see how this would not be part of the framework. I'm trying to keep the interdependence between the parent and child at a minimum. This will allow the parent to be built without the child keeping the code size small when a custom kernel config is needed.

Well, you have implemented some support for one kind of PICs. However, there could be more kinds of PICs and each of them may have own support. If we add some entries to struct pic for each of them, it will be very unfortunate. And this is what concerns me.

What about to add new method to pic_if.m to be called by child PIC and use intr_pic_add_handler() as generic implemetation. Define this structure

struct foo {
        struct mtx                 foo_lock;
        SLIST_HEAD(, intr_pic_child) foo_children;
};

to be used by parent PIC device and pass it to intr_child_irq_handler() as argument. Then it will work too and will really be independent. It can be in subr_intr.c as a support for some kind of PICs. And even if it's used by some PICs, it's not tied to the framework.

BTW, I have already discussed with mmel that there could be some support functions for simple PICs as their code is already very similar.

I'm happy to look at generalising the code in 12, however this is blocking moving arm64 to INTRNG. I'm going to commit this for now and look at reevaluating it in the future.