Design of interrupt controller driver

Design of interrupt controller driver

[Mason]

Hello,

My previous thread was a dud.

I will post a formal patch on Tuesday, but I would like
to clarify a few nagging doubts, the answer to which
have an impact on the driver's design.

1) The interrupt router has 128 inputs and 24 outputs.
Therefore, several devices have to share an output line.
I believe they *must* be of the same interrupt type?
In the limit, we could use
- 1 output line for level high
- 1 output line for level low
- 1 output line for edge rising
- 1 output line for edge falling
Is that correct?

2) Assume a device that signals an interrupt by pulsing
its interrupt line, i.e. the signal rises and then falls
a few cycles later. Assume we have grouped several such
devices on a single output line. When an interrupt triggers,
I see no way to determine which device requested attention
from the processor.
Does this mean that edge interrupts cannot be grouped
on a single output line for such a controller?


I have discussed this limitation with the HW designers.
They said "edge interrupts are not supported".
Looking at the current DT, I see two devices declared
with edge interrupts (DMA engine and ethernet PHY).

Looking more closely at the ethernet PHY description:

"Interrupt Signal to System; default OD-gate, needs an
external 10Kohm pull-up, active low; can be configured
to I/O by register, active high."

As maz points out, this sounds very level.
However, defining it as such in the DT hangs the system,
while it appears to work with edge rising. I don't know
how this is possible, but I have to investigate. Anyone
would care to venture a guess?


As for the DMA interrupt, the HW designers consider it
a level interrupt. When the engine is busy processing
a command, the interrupt signal is low; when the engine's
command queue is empty, the interrupt signal is high.
Thus, there is no risk of "missing a pulse". But maz
has stated twice that the relevant *event* is the
transition from "busy" to "idle", which makes this
an edge (rising) interrupt. However, it doesn't have
the problem described in 2)  The interrupt signal
will remain high, so there is no risk of "missing"
a pulse. So they can be grouped. Is that correct?


Regards.

Re: Design of interrupt controller driver

[Mason]

On 03/06/2017 18:49, Mason wrote:

> 1) The interrupt router has 128 inputs and 24 outputs.
> Therefore, several devices have to share an output line.
> I believe they *must* be of the same interrupt type?
> In the limit, we could use
> - 1 output line for level high
> - 1 output line for level low
> - 1 output line for edge rising
> - 1 output line for edge falling
> Is that correct?
>
> 2) Assume a device that signals an interrupt by pulsing
> its interrupt line, i.e. the signal rises and then falls
> a few cycles later. Assume we have grouped several such
> devices on a single output line. When an interrupt triggers,
> I see no way to determine which device requested attention
> from the processor.
> Does this mean that edge interrupts cannot be grouped
> on a single output line for such a controller?

A year ago, maz wrote:

> OK, so this is definitely a pure router, and the lack of latch makes 
> it completely unsuitable for a a cascaded interrupt controller. At 
> least, we've managed to establish that this thing will never be able 
> to handle more than 24 devices in a sane way. So let's forget about 
> Mason's idea of cascading everything to a single output line, and 
> let's focus on your initial idea of having something similar to TI's 
> crossbar, which is a much saner approach.
Then later added:

> Unless you limit your mux [to] level interrupts only, I cannot see how
> you could deal with cascaded interrupts. By the time you receive an
> edge, the line will have dropped, and you won't be able to identify
> the source interrupt.
To recap:
Sharing level interrupts would be OK.
Sharing "pulse" (rapid high/low) interrupts is impossible.
Sharing the DMA interrupts should be OK.

AFAIK, there are no devices using "pulses" in the system.

Will carefully re-read the July patch RFC.

Regards.

Re: Design of interrupt controller driver

[Thomas Gleixner]

On Sat, 3 Jun 2017, Mason wrote:
> 1) The interrupt router has 128 inputs and 24 outputs.
> Therefore, several devices have to share an output line.
> I believe they *must* be of the same interrupt type?
> In the limit, we could use
> - 1 output line for level high
> - 1 output line for level low
> - 1 output line for edge rising
> - 1 output line for edge falling
> Is that correct?

No. You CANNOT share edge type interrupts, simply because it might be
impossible to figure out which device fired the interrupt.

> As for the DMA interrupt, the HW designers consider it
> a level interrupt. When the engine is busy processing
> a command, the interrupt signal is low; when the engine's
> command queue is empty, the interrupt signal is high.
> Thus, there is no risk of "missing a pulse". But maz
> has stated twice that the relevant *event* is the
> transition from "busy" to "idle", which makes this
> an edge (rising) interrupt. However, it doesn't have
> the problem described in 2)  The interrupt signal
> will remain high, so there is no risk of "missing"
> a pulse. So they can be grouped. Is that correct?

That depends on the DMA controller. If you can disable the interrupt line
at the DMA device level, then you can use that scheme. If not, you'll get
an interrupt storm as long as the DAM engine is idle.

Thanks,

	tglx

Re: Design of interrupt controller driver

[Mason]

On 04/06/2017 15:55, Thomas Gleixner wrote:

> On Sat, 3 Jun 2017, Mason wrote:
>
>> 1) The interrupt router has 128 inputs and 24 outputs.
>> Therefore, several devices have to share an output line.
>> I believe they *must* be of the same interrupt type?
>> In the limit, we could use
>> - 1 output line for level high
>> - 1 output line for level low
>> - 1 output line for edge rising
>> - 1 output line for edge falling
>> Is that correct?
> 
> No. You CANNOT share edge type interrupts, simply because it might be
> impossible to figure out which device fired the interrupt.

I understand that now.
It dawned on me in point 2.

The interrupt controller's designers consider that
"pulse" interrupt requests are "broken by design",
because they can be missed if the edge detector is
sampling at too low a frequency.

>> As for the DMA interrupt, the HW designers consider it
>> a level interrupt. When the engine is busy processing
>> a command, the interrupt signal is low; when the engine's
>> command queue is empty, the interrupt signal is high.
>> Thus, there is no risk of "missing a pulse". But maz
>> has stated twice that the relevant *event* is the
>> transition from "busy" to "idle", which makes this
>> an edge (rising) interrupt. However, it doesn't have
>> the problem described in 2)  The interrupt signal
>> will remain high, so there is no risk of "missing"
>> a pulse. So they can be grouped. Is that correct?
> 
> That depends on the DMA controller. If you can disable the interrupt line
> at the DMA device level, then you can use that scheme. If not, you'll get
> an interrupt storm as long as the DMA engine is idle.

I don't follow. If the interrupt is configured
as rising edge, only transitions from busy to
idle will trigger an interrupt. The interrupt
storm would be if the interrupt is configured
level high. Right?

I'm using a DMA driver right now which expects
the ISR to be invoked on rising edges, and
everything works as expected.

Regards, and have a nice weekend.

Re: Design of interrupt controller driver

[Thomas Gleixner]

On Sun, 4 Jun 2017, Mason wrote:
> On 04/06/2017 15:55, Thomas Gleixner wrote:
> >> As for the DMA interrupt, the HW designers consider it
> >> a level interrupt. When the engine is busy processing
> >> a command, the interrupt signal is low; when the engine's
> >> command queue is empty, the interrupt signal is high.
> >> Thus, there is no risk of "missing a pulse". But maz
> >> has stated twice that the relevant *event* is the
> >> transition from "busy" to "idle", which makes this
> >> an edge (rising) interrupt. However, it doesn't have
> >> the problem described in 2)  The interrupt signal
> >> will remain high, so there is no risk of "missing"
> >> a pulse. So they can be grouped. Is that correct?
> > 
> > That depends on the DMA controller. If you can disable the interrupt line
> > at the DMA device level, then you can use that scheme. If not, you'll get
> > an interrupt storm as long as the DMA engine is idle.
> 
> I don't follow.

Sorry, hit send too early. Let me rephrase:

When you configure the interrupt as edge then you cannot share it. No
matter whether it stays high or not.

The only way to share it is, to configure it as level interrupt. But that
requires that you can disable the interrupt at the DMA device level once it
triggered. Otherwise you get an interrupt storm.

Thanks,

	tglx

Re: Design of interrupt controller driver

[Mason]

On 04/06/2017 22:13, Thomas Gleixner wrote:
> On Sun, 4 Jun 2017, Mason wrote:
>> On 04/06/2017 15:55, Thomas Gleixner wrote:
>>>> As for the DMA interrupt, the HW designers consider it
>>>> a level interrupt. When the engine is busy processing
>>>> a command, the interrupt signal is low; when the engine's
>>>> command queue is empty, the interrupt signal is high.
>>>> Thus, there is no risk of "missing a pulse". But maz
>>>> has stated twice that the relevant *event* is the
>>>> transition from "busy" to "idle", which makes this
>>>> an edge (rising) interrupt. However, it doesn't have
>>>> the problem described in 2)  The interrupt signal
>>>> will remain high, so there is no risk of "missing"
>>>> a pulse. So they can be grouped. Is that correct?
>>>
>>> That depends on the DMA controller. If you can disable the interrupt line
>>> at the DMA device level, then you can use that scheme. If not, you'll get
>>> an interrupt storm as long as the DMA engine is idle.
>>
>> I don't follow.
> 
> Sorry, hit send too early. Let me rephrase:
> 
> When you configure the interrupt as edge then you cannot share it. No
> matter whether it stays high or not.

Could you explain why? (I must be missing something.)

If the signal is low when the controller is busy,
and high when the controller is idle, then the
transition from busy to idle is a rising edge,
which will trigger an interrupt. The intc driver
will look up the status of all devices, see that
the DMA engine interrupt request line is high,
and thus call the DMA engine's ISR.

I have that setup right now, sharing TX and RX
interrupts, and the device using the DMA engine
(a NAND Flash controller) works well.

Is there a problem if several shared edge interrupts
fire at the same time (or very close to one another).
I don't see the problem, as long as the line stays
high until software does something with the device.

> The only way to share it is, to configure it as level interrupt. But that
> requires that you can disable the interrupt at the DMA device level once it
> triggered. Otherwise you get an interrupt storm.

I'm not sure what you mean with "disable the interrupt at the
DMA device level". The interrupt can be masked at the system
interrupt controller (i.e. before sharing the interrupt
signal). The DMA engine just outputs 0 when busy, 1 when idle.

Regards.

Re: Design of interrupt controller driver

[Thomas Gleixner]

On Mon, 5 Jun 2017, Mason wrote:
> On 04/06/2017 22:13, Thomas Gleixner wrote:
> > When you configure the interrupt as edge then you cannot share it. No
> > matter whether it stays high or not.
> 
> Could you explain why? (I must be missing something.)

Device A    Device B	 Combined Output  Edge detection
Low    	    Low	   	 0	  	  N

Low -> High Low		 1	    	  Y  -> Interrupt handled

High   	    Low -> High	 1	    	  N

When the A line stays high, which it does, then the edge detector will not
see a transition for B and you lose an interrupt.

> > The only way to share it is, to configure it as level interrupt. But that
> > requires that you can disable the interrupt at the DMA device level once it
> > triggered. Otherwise you get an interrupt storm.
> 
> I'm not sure what you mean with "disable the interrupt at the
> DMA device level". The interrupt can be masked at the system
> interrupt controller (i.e. before sharing the interrupt
> signal). The DMA engine just outputs 0 when busy, 1 when idle.

Sharing level interrupts requires a way to disable the device (in your case
the DMA engine) interrupt output in order to prevent irq storms.

Pseudo code (locking etc. omitted):

irq_handler_devA()
{
	if (!interrupt_active(devA))
		return IRQ_NONE;

	handle_device_irq();

	if (no_more_outstanding_requests(devA)) {
		reg = readl(devA->irq_control_reg);
		reg &= ~DEV_IRQ_ENABLE;
		writel(devA->irq_control_reg, reg);
	}
	return IRQ_HANDLED;
}

queue_reqeust_devA()
{
	if (no_more_outstanding_requests(devA)) {
		queue_request();
		
		start_engine();

		/* Reenable interrupt at device level */
		reg = readl(devA->irq_control_reg);
		reg |= DEV_IRQ_ENABLE;
		writel(devA->irq_control_reg, reg);
	} else {
	       queue_request();
	}
}

You get the idea.

Thanks,

	tglx

Re: Design of interrupt controller driver

[Mason]

On 05/06/2017 10:23, Thomas Gleixner wrote:
> On Mon, 5 Jun 2017, Mason wrote:
>> On 04/06/2017 22:13, Thomas Gleixner wrote:
>>> When you configure the interrupt as edge then you cannot share it. No
>>> matter whether it stays high or not.
>>
>> Could you explain why? (I must be missing something.)
> 
> Device A    Device B	 Combined Output  Edge detection
> Low    	    Low	   	 0	  	  N
> 
> Low -> High Low		 1	    	  Y  -> Interrupt handled
> 
> High   	    Low -> High	 1	    	  N
> 
> When the A line stays high, which it does, then the edge detector will not
> see a transition for B and you lose an interrupt.

Doh! It's totally obvious, now that you point it out.
I must be mis-remembering my previous setup.
I'll review everything before submitting a patch.

>>> The only way to share it is, to configure it as level interrupt. But that
>>> requires that you can disable the interrupt at the DMA device level once it
>>> triggered. Otherwise you get an interrupt storm.
>>
>> I'm not sure what you mean with "disable the interrupt at the
>> DMA device level". The interrupt can be masked at the system
>> interrupt controller (i.e. before sharing the interrupt
>> signal). The DMA engine just outputs 0 when busy, 1 when idle.
> 
> Sharing level interrupts requires a way to disable the device (in your case
> the DMA engine) interrupt output in order to prevent irq storms.
> 
> Pseudo code (locking etc. omitted):
> 
> irq_handler_devA()
> {
> 	if (!interrupt_active(devA))
> 		return IRQ_NONE;
> 
> 	handle_device_irq();
> 
> 	if (no_more_outstanding_requests(devA)) {
> 		reg = readl(devA->irq_control_reg);
> 		reg &= ~DEV_IRQ_ENABLE;
> 		writel(devA->irq_control_reg, reg);
> 	}
> 	return IRQ_HANDLED;
> }
> 
> queue_request_devA()
> {
> 	if (no_more_outstanding_requests(devA)) {
> 		queue_request();
> 		
> 		start_engine();
> 
> 		/* Reenable interrupt at device level */
> 		reg = readl(devA->irq_control_reg);
> 		reg |= DEV_IRQ_ENABLE;
> 		writel(devA->irq_control_reg, reg);
> 	} else {
> 	       queue_request();
> 	}
> }
> 
> You get the idea.

I'll take a closer look at the DMA engine driver.

Is it possible to call the interrupt controller's mask callback
from the DMA engine driver ISR, or is that reserved for the IRQ
framework? Because that's what the HW designers had in mind,
thus they didn't provide a way to mask the interrupt in the
device. It just outputs the signal to the interrupt router.

So their proposed setup is as follows:

Start the system with the DMA interrupt masked in the intc.
When SW needs to perform a DMA op, the DMA driver starts
the op (thus the interrupt signal goes low), then unmasks
the interrupt in the intc. Interrupt triggers when signal
goes high (level high). Driver masks interrupt in ISR,
until next op is available.

Is that possible in the Linux framework?

Regards.

Re: Design of interrupt controller driver

[Thomas Gleixner]

On Mon, 5 Jun 2017, Mason wrote:
> Is it possible to call the interrupt controller's mask callback
> from the DMA engine driver ISR, or is that reserved for the IRQ
> framework? Because that's what the HW designers had in mind,
> thus they didn't provide a way to mask the interrupt in the
> device. It just outputs the signal to the interrupt router.

No, it's not and we are not going to provide an interface for that because
that violates any form of layering and abstractions. The DMA device driver
has to be oblivious of the underlying interrupt handling machinery.

> So their proposed setup is as follows:
> 
> Start the system with the DMA interrupt masked in the intc.
> When SW needs to perform a DMA op, the DMA driver starts
> the op (thus the interrupt signal goes low), then unmasks
> the interrupt in the intc. Interrupt triggers when signal
> goes high (level high). Driver masks interrupt in ISR,
> until next op is available.
> 
> Is that possible in the Linux framework?

You can do that, but not for shared interrupts:

isr()
{
	disable_irq_nosync();
	.....
}

submit()
{
	....
	enable_irq();
}

init()
{
	irq_set_status_flags(irq, IRQ_NOAUTOEN);
	request_irq(irq, ....);
}

This affects the whole interrupt line, so if you share that interrupt at
the CPU interrupt controller level this wont work.

If your 'router' IP has a mechanism to mask input lines individually, then
you can do something about this.

Each group of inputs which shares an output becomes it's own demultiplexing
interrupt domain with it's own interrupt chip which controls the input
lines. The output is handled by a chained interrupt handler which checks
the status of the input lines and invokes the handlers for the devices with
an active input.

Then the above example will disable the interrupt at the 'router' level
which will not affect the other device which shares the underlying output
to the GIC (or whatever interrupt controller your CPU has).

Thanks,

	tglx

Re: Design of interrupt controller driver

[Mason]

On 06/06/2017 09:39, Thomas Gleixner wrote:

> On Mon, 5 Jun 2017, Mason wrote:
>
>> Is it possible to call the interrupt controller's mask callback
>> from the DMA engine driver ISR, or is that reserved for the IRQ
>> framework? Because that's what the HW designers had in mind,
>> thus they didn't provide a way to mask the interrupt in the
>> device. It just outputs the signal to the interrupt router.
> 
> No, it's not and we are not going to provide an interface for that because
> that violates any form of layering and abstractions. The DMA device driver
> has to be oblivious of the underlying interrupt handling machinery.

Sorry, I didn't mean *explicitly* calling the mask callback,
but rather void mask_irq(struct irq_desc *desc);
I note that mask_irq() is *not* exported, which means modules
cannot to use it. In-tree drivers are probably not supposed
to use it either? Same thing for irq_disable?

What about disable_irq(virq);
That function /is/ exported API, and eventually calls mask_irq.

disable_irq -> __disable_irq_nosync -> __disable_irq -> irq_disable -> mask_irq

>> So their proposed setup is as follows:
>>
>> Start the system with the DMA interrupt masked in the intc.
>> When SW needs to perform a DMA op, the DMA driver starts
>> the op (thus the interrupt signal goes low), then unmasks
>> the interrupt in the intc. Interrupt triggers when signal
>> goes high (level high). Driver masks interrupt in ISR,
>> until next op is available.
>>
>> Is that possible in the Linux framework?
> 
> You can do that, but not for shared interrupts:
> 
> isr()
> {
> 	disable_irq_nosync();
> 	.....
> }
> 
> submit()
> {
> 	....
> 	enable_irq();
> }
> 
> init()
> {
> 	irq_set_status_flags(irq, IRQ_NOAUTOEN);
> 	request_irq(irq, ....);
> }
> 
> This affects the whole interrupt line, so if you share that interrupt at
> the CPU interrupt controller level this wont work.
> 
> If your 'router' IP has a mechanism to mask input lines individually, then
> you can do something about this.
> 
> Each group of inputs which shares an output becomes it's own demultiplexing
> interrupt domain with it's own interrupt chip which controls the input
> lines. The output is handled by a chained interrupt handler which checks
> the status of the input lines and invokes the handlers for the devices with
> an active input.
> 
> Then the above example will disable the interrupt at the 'router' level
> which will not affect the other device which shares the underlying output
> to the GIC (or whatever interrupt controller your CPU has).

I will have to re-read this a few times to digest it.

Regards.

Re: Design of interrupt controller driver

[Thomas Gleixner]

On Tue, 6 Jun 2017, Mason wrote:
> On 06/06/2017 09:39, Thomas Gleixner wrote:
> What about disable_irq(virq);
> That function /is/ exported API, and eventually calls mask_irq.
> 
> disable_irq -> __disable_irq_nosync -> __disable_irq -> irq_disable -> mask_irq

What you can call from an ISR is disable_irq_nosync(). disable_irq() will
deadlock.

Thanks,

	tglx