Re: [RFC PATCH V2 2/8] irqdomain: Don't set type when mapping an IRQ

[Geert Uytterhoeven]

On Tue, Dec 22, 2015 at 12:56 PM, Jon Hunter <jonathanh@xxxxxxxxxx> wrote:
> On 22/12/15 11:18, Grygorii Strashko wrote:
>> On 12/17/2015 12:48 PM, Jon Hunter wrote:
>>> Some IRQ chips, such as GPIO controllers or secondary level interrupt
>>> controllers, may require require additional runtime power management
>>> control to ensure they are accessible. For such IRQ chips, it makes sense
>>> to enable the IRQ chip when interrupts are requested and disabled them
>>> again once all interrupts have been freed.
>>>
>>> When mapping an IRQ, the IRQ type settings are read and then programmed.
>>> The mapping of the IRQ happens before the IRQ is requested and so the
>>> programming of the type settings occurs before the IRQ is requested. This
>>> is a problem for IRQ chips that require additional power management
>>> control because they may not be accessible yet. Therefore, when mapping
>>> the IRQ, don't program the type settings, just save them and then program
>>> these saved settings when the IRQ is requested (so long as if they are not
>>> overridden via the call to request the IRQ).
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@xxxxxxxxxx>

>> This patch has side effect - some boards which have
>> ARM TWD timer will produce below backtrace on boot:
>>
>>  genirq: Setting trigger mode 4 for irq 17 failed (gic_set_type+0x0/0x58)
>>  twd: can't register interrupt 17 (-22)
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 0 at arch/arm/kernel/smp_twd.c:405 twd_local_timer_of_register+0x68/0x7c()

>> This happens, most probably, because TWD IRQs definitions in DT
>> do not corresponds HW and gic_configure_irq() will return -EINVAL
>> example (am4372.dtsi):
>>       local_timer: timer@48240600 {
>>               compatible = "arm,cortex-a9-twd-timer";
>>               reg = <0x48240600 0x100>;
>>               interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
>>               interrupt-parent = <&gic>;
>>               clocks = <&mpu_periphclk>;
>>               status = "disabled";
>>       };
>
>
> Hmm ... that's interesting. Looking at the GIC documentation, it does
> say that it is "implementation defined" whether you can program the type
> bit for a PPI. Therefore, I am wondering if we should convert the error
> into a WARN instead? It would be hard to know which of the below would
> be impacted from just looking at the DT files.
>
>> So, some additional fixes might be required. Potentially problematic files are:

>> arch/arm/boot/dts/r8a7779.dts
>> arch/arm/boot/dts/sh73a0.dtsi

The above two are affected. Sending patches...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds