Message-ID: <b5e0ff57-885a-051b-4c4c-a02b005fa1f1@cogentembedded.com>
Date:   Tue, 28 Dec 2021 22:03:23 +0300
From:   Nikita Yushchenko <nikita.yoush@...entembedded.com>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Magnus Damm <magnus.damm@...il.com>,
        linux-renesas-soc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH/RFC] drivers/irqchip: add irq-inverter

Hi

>> Interrupt trigger type is typically used to configure interrupt
>> controller to properly interpret interrupt signal sent from a device.
>>
>> However, some devices have configureable interrupt outputs, and drivers
>> tend to use interrupt trigger type also to configure device interrupt
>> output.
>>
>> This works well when device interrupt output is connected directly to
>> interrupt controller input. However, this is not always the case.
>> Sometimes the interrupt signal gets inverted between the device
>> producing it and the controller consuming it. Combined with both sides
>> using the same interrupt trigger type to configure the signal, this
>> results into non-working setup regardless of what interrupt trigger type
>> is configured.
> 
> Regardless? Surely there is a canonical, working configuration.

It is working as long as either hardware delivers interrupt signal without inversion, or only one side 
(producer or consumer) is configured while the other side stays constant.

It does not work when hardware inverts singnal and both producer and consumer are configured.

>> Usage example, for Kingfisher extension board for Renesas Gen-3 Soc,
>> that has WiFi interrupt delivered over inverting level-shifter:
>>
>> / {
>> 	wlcore_interrupt: inverter {
>> 		compatible = "linux,irq-inverter";
>> 		interrupts-extended = <&gpio1 25 IRQ_TYPE_EDGE_RISING>;
>> 		interrupt-controller;
>> 		#interrupt-cells = <0>;
>> 	};
>> };
>>
>> &wlcore {
>> 	interrupts-extended = <&wlcore_interrupt>;
>> };
> 
> So you don't describe the trigger at the endpoint level, but at the
> pseudo-interrupt controller level? /me feels mildly sick.

Could you please explain how this could be done?

Regardless of what is configured at endpoint side, interrupt controller driver will use that to set up 
interrupt controller, and wl18xxx driver (in the case) will use that to configure wl18xx. That results 
into SAME settings at producer and consumer sides, and hardware requires OPPOSITE sittings at producer 
and consumer sides.

It is not a problem in interrupt controller driver - that driver does it's job correctly, setting up the 
interrupt type that is requested.

It is likely not a problem in interrupt source (i.e. wl18xx) driver - that driver tries to set up it's 
irq in the way that will work with any interrupt controller. Perhaps it can be possible to update wl18xx 
driver to allow fixed setup of interrupt polarity, but that looks like addressing problem at wrong 
location. It is not an issue with wl18xx. There are quite a few drivers in the tree that setup interrupt 
polarity for their devices based on what irq_get_trigger_type() returns.

The root cause if the issue is the board itself, that inverts the signal. Thus it looks correct to tie 
the fix to the board. And a device node describing the interconnect looks like an elegant solution for this.

> And by the way: "An interrupt specifier is one or more cells of data
> (as specified by #interrupt-cells) ...". Ergo, #interrupt-cells cannot
> be 0 when the interrupt controller can be an interrupt-parent.

Code works with #interrupt-cells=0 correctly, as long as interrupts-extended property is used at 
producer side.

>> Then, wl18xx driver gets IRQ_TYPE_EDGE_FALLING return from
>> irq_get_trigger_type() call, and configures interrupt output for that.
>> Then the signal is delivered inverted to the GPIO module, and handled
>> correctly, because GPIO is configured for IRQ_TYPE_EDGE_RISING.
> 
> So this is only to avoid writing the correct device tree?

No. It is an attempt to describe a case that seems to be not currently describable.

Vendor BSPs solve this by commenting out irq polarity setup in drivers. Thus obviously breaking use 
cases other than these BSPs are for. I'm trying to suggest a portable alternative instead.

>> - why not using hierarchial irq_domain?
>> - because with hierarchial irq_domain, same interrupt gets the same virq
>>    number at all levels, and trigger type is tied to virq number, so need
>>    different virq numbers for reporting different trigger types
> 
> Why would you have different interrupt numbers? A given line has one
> configuration at any given point, and only one.

The goal is - make irq_get_trigger_type() returning different values for producer and consumer of the 
interrupt. Because, that matches the hardware behavior.

While irq_get_trigger_type() is used for that, returning different values for producer and consumer is 
obviously impossible.

Originally, I was considering to add a different API to use by interrupt producer instead of 
irq_get_trigger_type(), to deliver information configured by additional flag in DT node for interrupt 
producer. I.e.
     interrupts = <25 IRQ_TYPE_EDGE_RISING | IRQ_INVERTED_ROUTE>

But, inverted route is not a feature of interrupt, it is a feature of connection. I.e. nothing stops 
from having several sources routed to the same interrupt, and having only one of these sources inverted. 
This means, the IRQ_INVERTED_ROUTE flag is not interrupt's flag but connection's flag. And, a virtual 
irqchip looked like a good abstraction to describe connection.

The fact that this make the entire solution much smaller, was just a pleasant side-effect ;).

>> - why using request_irq() for parent irq, instead of setting up chained
>>    interrupt in irqchips?
>> - because this way code is much simpler, and shall work for all cases
>>    (such as normal/threaded parent irq, normal/threaded child irq,
>>    different parent interrupt chips, etc)
> 
> And that's a NAK for deliberately violating the irqchip API.

I've learned the idea of calling generic_handle_irq() from interrupt handler from 
Documentation/driver-api/gpio/driver.rst

It looked as an elegant solution to avoid complexity (such as: a chained interrupt is activated at 
registration time, assuming there is a piece of hardware [chained controller] that will avoid interrupt 
firing until a chained source fires...  but for pure-software chained interrupt, must keep parent 
disabled up to when a handler for child is installed)

But, I definitely don't insist on this approach.

>> - why just not introducing separate API for consumer-side and
>>    produced-side trigger type?
>> - because with the chosen approach, no changes are needed to any cases
>>    that don't suffer from inverted interrupt routing
> 
> The right way to do it is to use the existing API by exposing the
> inverter (there are existing examples in the tree, using the
> hierarchical model). It isn't rocket science, and not much more code
> than the pile of hacks^W^W^Wcreative approach you have.

Could you please point me to such examples? I could not find any.

Thanks,

Nikita

Powered by blists - more mailing lists