[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aB230INCy2h7X1KY@lpieralisi>
Date: Fri, 9 May 2025 10:07:44 +0200
From: Lorenzo Pieralisi <lpieralisi@...nel.org>
To: Marc Zyngier <maz@...nel.org>
Cc: Thomas Gleixner <tglx@...utronix.de>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>, Arnd Bergmann <arnd@...db.de>,
Sascha Bischoff <sascha.bischoff@....com>,
Timothy Hayes <timothy.hayes@....com>,
"Liam R. Howlett" <Liam.Howlett@...cle.com>,
Mark Rutland <mark.rutland@....com>,
Jiri Slaby <jirislaby@...nel.org>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org
Subject: Re: [PATCH v3 20/25] irqchip/gic-v5: Add GICv5 PPI support
On Thu, May 08, 2025 at 12:44:45PM +0200, Lorenzo Pieralisi wrote:
[...]
> I noticed that, if the irq_set_type() function is not implemented,
> we don't execute (in __irq_set_trigger()):
>
> irq_settings_set_level(desc);
> irqd_set(&desc->irq_data, IRQD_LEVEL);
I don't get why the settings above are written only if the irqchip
has an irq_set_type() method, maybe they should be updated in
irqdomain code (?) where:
irqd_set_trigger_type()
is executed after creating the fwspec mapping ?
Is it possible we never noticed because we have always had irqchips that
do implement irq_set_type() ?
Again, I don't know the history behind the IRQD_LEVEL flag so it is just
a question, I'd need to get this clarified though please if I remove the
PPI irq_set_type() callback.
Thanks,
Lorenzo
> which in turn means that irqd_is_level_type(&desc->irq_data) is false
> for PPIs (ie arch timers, despite being level interrupts).
>
> An immediate side effect is that they show as edge in:
>
> /proc/interrupts
>
> but that's just what I could notice.
>
> Should I set them myself in PPI translate/alloc functions ?
>
> Removing the irq_set_type() for PPIs does not seem so innocuous, it is a
> bit complex to check all ramifications, please let me know if you spot
> something I have missed.
>
> > > On the other hand, given that on GICv5 PPI handling mode is fixed,
> > > do you think that in the ppi_irq_domain_ops.translate() callback,
> > > I should check the type the firmware provided and fail the translation
> > > if it does not match the HW hardcoded value ?
> >
> > Why? The fact that the firmware is wrong doesn't change the hardware
> > integration. It just indicates that whoever wrote the firmware didn't
> > read the documentation.
> >
> > Even more, I wonder what the benefit of having that information in the
> > firmware tables if the only thing that matters in the immutable HW
> > view. Yes, having it in the DT/ACPI simplifies the job of the kernel
> > (only one format to parse). But it is overall useless information.
>
> Yes, that I agree but it would force firmware bindings to special case
> PPIs to remove the type (#interrupt-cells and co.).
>
> From what I read I understand I must ignore the PPI type provided by
> firmware.
>
> > > Obviously if firmware exposes the wrong type that's a firmware bug
> > > but I was wondering whether it is better to fail the firmware-to-Linux
> > > IRQ translation if the firmware provided type is wrong rather than carry
> > > on pretending that the type is correct (I was abusing the irq_set_type()
> > > callback to do just that - namely, check that the type provided by
> > > firmware matches HW but I think that's the wrong place to put it).
> >
> > I don't think there is anything to do. Worse case, you spit a
> > pr_warn_once() and carry on.
>
> Thanks,
> Lorenzo
Powered by blists - more mailing lists