[PATCH RFC] irq-bcm2836: Avoid "Invalid trigger warning"

[Stefan Wahren]

> Marc Zyngier <marc.zyngier at arm.com> hat am 16. November 2017 um 19:16 geschrieben:
> 
> 
> On 16/11/17 17:53, Phil Elwell wrote:
> > Hi Stefan,
> > 
> > On 16/11/2017 17:34, Stefan Wahren wrote:
> >> Hi Phil,
> >>
> >>> Marc Zyngier <marc.zyngier at arm.com> hat am 16. November 2017 um 09:57 geschrieben:
> >>>
> >>>
> >>> On Thu, Nov 16 2017 at  7:53:02 am GMT, Stefan Wahren <stefan.wahren at i2se.com> wrote:
> >>>> From: Phil Elwell <phil at raspberrypi.org>
> >>>>
> >>>> Initialise the level for each IRQ to avoid a warning from the
> >>>> arm arch timer code:
> >>>>
> >>>>     arch_timer: WARNING: Invalid trigger for IRQ19, assuming level low
> >>>>     arch_timer: WARNING: Please fix your firmware
> >>>>     arch_timer: cp15 timer(s) running at 19.20MHz (virt).
> >>>>
> >>>> Signed-off-by: Phil Elwell <phil at raspberrypi.org>
> >>>> Signed-off-by: Stefan Wahren <stefan.wahren at i2se.com>
> >>>> ---
> >>>>  drivers/irqchip/irq-bcm2836.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/irqchip/irq-bcm2836.c b/drivers/irqchip/irq-bcm2836.c
> >>>> index 667b9e1..abc9b40 100644
> >>>> --- a/drivers/irqchip/irq-bcm2836.c
> >>>> +++ b/drivers/irqchip/irq-bcm2836.c
> >>>> @@ -104,7 +104,7 @@ static void bcm2836_arm_irqchip_register_irq(int hwirq, struct irq_chip *chip)
> >>>>  
> >>>>  	irq_set_percpu_devid(irq);
> >>>>  	irq_set_chip_and_handler(irq, chip, handle_percpu_devid_irq);
> >>>> -	irq_set_status_flags(irq, IRQ_NOAUTOEN);
> >>>> +	irq_set_status_flags(irq, IRQ_NOAUTOEN | IRQ_TYPE_LEVEL_LOW);
> >>>>  }
> >>>>  
> >>>>  static void
> >>>
> >>> Why is this only done for the per-cpu interrupts? I can't see what
> >>> guarantees the same thing for global interrupts...
> >>
> >> i don't know. Could you please answer?
> >>
> >> I'm only interested to get the rid of this ugly warning ...
> >>
> >> and the right fix ;-)
> > 
> > That was my motivation too, with a preference for the smallest change that had
> > the desired effect.
> > 
> > The warning message comes from the arch_timer code, which only cares about the
> > interrupts it has been configured to use. Since these are all per-cpu
> > interrupts, the state of the global interrupts is irrelevant.
> 
> The fact that no driver screams about it doesn't make it right. You're
> being bitten by the lack of interrupt polarity description in your DT.
> That is fine as long as you only support one type, but you need to tell
> the core code what you're doing.
> 
> Your whole interrupt registration thing is already quite a departure
> from the normal flow, where the interrupt should be created via the DT
> parsing code, and not eagerly like it is done here. I'd rather you
> address it by using the expected flow for a DT platform, with a xlate
> method that returns the actual trigger type (I assume all interrupts on
> this system are level...).
> 
> The same issue is present in the bcm2835 interrupt controller, which
> seems to be the one implementing global interrupts.

Okay, thanks for the explanation.

Can you please point me to a good reference implementation?

> 
> Thanks,
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...