diff mbox

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

Message ID 1510815182-4889-1-git-send-email-stefan.wahren@i2se.com (mailing list archive)
State New, archived
Headers show

Commit Message

Stefan Wahren Nov. 16, 2017, 6:53 a.m. UTC 
From: Phil Elwell <phil@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@raspberrypi.org>
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/irqchip/irq-bcm2836.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Marc Zyngier Nov. 16, 2017, 8:57 a.m. UTC | #1 
On Thu, Nov 16 2017 at  7:53:02 am GMT, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> From: Phil Elwell <phil@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@raspberrypi.org>
> Signed-off-by: Stefan Wahren <stefan.wahren@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...

Thanks,

	M.
Stefan Wahren Nov. 16, 2017, 5:34 p.m. UTC | #2 
Hi Phil,

> Marc Zyngier <marc.zyngier@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@i2se.com> wrote:
> > From: Phil Elwell <phil@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@raspberrypi.org>
> > Signed-off-by: Stefan Wahren <stefan.wahren@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 ;-)

Stefan

> 
> Thanks,
> 
> 	M.
> -- 
> Jazz is not dead, it just smell funny.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Phil Elwell Nov. 16, 2017, 5:53 p.m. UTC | #3 
Hi Stefan,

On 16/11/2017 17:34, Stefan Wahren wrote:
> Hi Phil,
> 
>> Marc Zyngier <marc.zyngier@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@i2se.com> wrote:
>>> From: Phil Elwell <phil@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@raspberrypi.org>
>>> Signed-off-by: Stefan Wahren <stefan.wahren@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.

Phil
Marc Zyngier Nov. 16, 2017, 6:16 p.m. UTC | #4 
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@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@i2se.com> wrote:
>>>> From: Phil Elwell <phil@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@raspberrypi.org>
>>>> Signed-off-by: Stefan Wahren <stefan.wahren@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.

Thanks,

	M.
Stefan Wahren Nov. 16, 2017, 6:42 p.m. UTC | #5 
> Marc Zyngier <marc.zyngier@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@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@i2se.com> wrote:
> >>>> From: Phil Elwell <phil@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@raspberrypi.org>
> >>>> Signed-off-by: Stefan Wahren <stefan.wahren@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...
diff mbox

Patch

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