Re: [PATCH v2] drivers: irqchip: add irq-type-changer

[Marc Zyngier]

On Tue, 25 Jan 2022 05:47:37 +0000,
Nikita Yushchenko <nikita.yoush@xxxxxxxxxxxxxxxxxx> wrote:
> 
> Hi
> 
> > I also don't see why you model this as the actual device that triggers
> > the interrupt.
> 
> Well, that somehow matches the physical reality. In the case of wl18xx
> on KF, physically the interrupt signal indeed originates from the
> intermediate chip - the inverting level-shifter.

Reality? By allowing something like an edge-to-level conversion? How
can that work?

> 
> I remember your suggestion to configure interrupt source's node with
> interrupt-parent set to inverter and interrupt details for inverter's
> parent. But this looks hacky and inconsistent for me.

We'll have to agree to disagree.

> 
> In contrary, an abstraction of intermediate entity that does a static
> conversion of the trigger type and does not need any software control,
> looks sane. And, hardware designers do strange things sometimes, I
> won't be surprised observing a change from level to edge one day.

If you think that it can happen without a HW register that latches the
edge and requires an ack, you need to question your understanding of
an interrupt life cycle, and of the properties of the various trigger
types.

> Thus it looked a good idea not to limit the conversion to inversion,
> but support arbitrary one. Then, irqspec inside the node for the
> intermediate entity obtains a meaning, making the entire model
> consistent.

Again, this cannot work, because the very *semantics* of an edge
interrupt (event) cannot directly be converted in a level (state).

> 
> I don't strongly insist on this approach, it just looks cleaner for me.
> 
> 
> > I'm also pretty sure that with the current code,
> > you end-up with *two* interrupts (one for the inverter and one for the
> > end-point).
> 
> In driver's init, I only call of_irq_parse_one() for interrupt defined
> in the changer's node. This does not create a mapping for it. The
> mapping is only created when changer's "interrupt-child" creates a
> mapping for their interrupt - then changer's alloc() routine calls
> irq_domain_alloc_irqs_parent() in the same way as all other
> hierarchical irqchips do.
> 
> I don't see where double mapping can appear here. Please explain.

Just look at your code. You start probing a device that has an
'interrupts' property. This triggers the allocation of an interrupt.
Then the endpoint also has an 'interrupts' property, also allocating
an interrupt. You then happily override the mapping of the first IRQ
with the second one in the parent irqchip.

That's just broken.

	M.

-- 
Without deviation from the norm, progress is not possible.