Re: [PATCH V3 1/5] genirq: define flag IRQ_SRC_DST_INVERTED, and accessors

[Thomas Gleixner]

On Tue, 4 Mar 2014, Stephen Warren wrote:

> On 03/04/2014 03:04 AM, Thomas Gleixner wrote:
> > On Mon, 3 Mar 2014, Stephen Warren wrote:
> > 
> >> From: Stephen Warren <swarren@xxxxxxxxxx>
> >>
> >> Some devices have configurable IRQ output polarities. Software might
> >> use IRQ_TYPE_* to determine how to configure such a device's IRQ
> >> output polarity in order to match how the IRQ controller input is
> >> configured. If the board or SoC inverts the signal between the
> >> device's IRQ output and controller's IRQ output, software must be
> >> aware of this fact, in order to program the IRQ output to the correct
> >> (i.e. opposite) polarity. This flag provides that information.
> > 
> > So what you're saying is:
> > 
> > Device IRQ output --> [Optional Inverter Logic] --> IRQ controller input.
> > 
> > And you're storing the information about the presence of the inverter
> > logic in the irq itself, but the core does not make any use of it and
> > you let the device driver deal with the outcome.
> > 
> > This sucks as all affected drivers have to implement the same sanity
> > logic for this.
> > 
> > Why don't you implement a core function which tells the driver which
> > polarity to select? That requires a few more changes, but I think it's
> > worth it for other reasons.
> > 
> > Right now the set_type logic requires the irq chip drivers to
> > implement sanity checking and default selections for TYPE_NONE. We can
> > be more clever about that and add this information to the irq chip
> > flags.
> 
> I don't see any such checking in drivers/irqchip/irq-gic.c; it rejects
> any type other than IRQ_TYPE_LEVEL_HIGH or IRQ_TYPE_EDGE_RISING, and I
> don't see any mention of TYPE_NONE in that file. Is the driver incomplete?

No. The IRQ_TYPE_NONE is a hysterical leftover.
 
> Instead of adding all this extra logic to the core, what do you think of
> simply telling each driver that has a configurable interrupt output
> polarity exactly which polarity to use. This information would come from
> device tree or platform data. This is what I implemented in V1/V2 of
> this series:
>
> http://www.spinics.net/lists/devicetree/msg23648.html
> 
> Is that any better, or do you definitely want the IRQ core to manage this?

Oh, yes.

Simply because any driver which is not aware of that inversion will
trip over the issue that it requests by the best of its knowledge
IRQ_TYPE_LEVEL_HIGH while it should actually request
IRQ_TYPE_LEVEL_LOW due to the inversion.

Are you really going to make all possibly affected drivers aware of
that? Good luck!

That's why we want to move such stuff to core code. Assume the
following scenario:

    driverX which works perfectly fine on SoCA is reused for SoCB
    where the inverter sits between the device and the SoCB irq
    controller.

With your DT scheme the whole thing falls flat on the nose simply
because neither the driverX nor the core code is aware of the
incompability.

So driverX is happily using the IRQ_TYPE_LEVEL_HIGH setting which was
used when the driver was written and the core works nicely with that
because the irq chips supports IRQ_TYPE_LEVEL_HIGH. Just the fact that
the inverter is in the hardware results in an infinite interrupt
storm. Are you going to handle the bug reports and "remote" debug
sessions for this kind of crap?

Fuck no. You don't want to deal with this and that's why it is way
better to build that kind of support into the core where everything
which trips over the issue tells the random driver user/developer
what's wrong.

Dammit. I explained you very detailed WHY this is useful aside of the
inverted logic situation. Is it that hard to understand?

This usecase clearly shows a shortcoming at the core level along with
a potential for consolidation. So why are you trying to convince me
that this can be solved with some DT/device drivers hackery?

When will you SoC folks ever understand that nothing on your
SoCs/board is special? I'm telling that you for years but you simply
refuse to get a gripe.

Thanks,

	tglx



 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/