Re: dmtimer: ack pending interrupt during suspend on am335x/am437x?

From: Marc Zyngier <maz@kernel.org>
To: Drew Fustini <dfustini@baylibre.com>
Cc: Tony Lindgren <tony@atomide.com>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Dave Gerlach <d-gerlach@ti.com>,
	linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: dmtimer: ack pending interrupt during suspend on am335x/am437x?
Date: Tue, 10 May 2022 09:16:34 +0100	[thread overview]
Message-ID: <7c43bcfd56424d909796977460335f24@misterjones.org> (raw)
In-Reply-To: <YniiqM0S+hwsGFni@x1>

On 2022-05-09 06:12, Drew Fustini wrote:
> Hello Daniel, Tony suggested I mail you along with the list to get
> feedback. I'm attempting to upstream these two patches [1][2] from
> ti-linux-5.4.y for arch/arm/mach-omap2/timer.c:
> 96f4c6e2ba8a ("ARM: OMAP2+: timer: Ack pending interrupt during 
> suspend")
> 7ae7dd5f8272 ("ARM: OMAP2+: timer: Extend pending interrupt ACK for 
> gic")
> 
> On the TI AM335x and AM437x SoCs, it is possible for a late interrupt 
> to
> be generated which will cause a suspend failure. The first patch makes
> omap_clkevt_idle() ack the irq both in the timer peripheral register
> and in the interrupt controller to avoid the issue.
> 
> On AM437x only, the GIC cannot be directly acked using only the irqchip
> calls. To workaround that, the second patch maps the GIC_CPU_BASE and
> reads the GIC_CPU_INTACK register before calling irq_eoi to properly 
> ack
> the late timer interrupts that show up during suspend.

This isn´t an Ack. The Ack happens when you read the IAR register
(Interrupt Acknowledgement Register). Writing to EOI performs at least
a priority drop, and maybe a deactivation.

Simply writing to EOI doesn´t necessarily solve any problem if the
GIC is using EOIMode==1, because you´ĺl miss the deactivation.

> 
> However, Tony removed most of arch/arm/mach-omap2/timer.c with:
> 2ee04b88547a ("ARM: OMAP2+: Drop old timer code for dmtimer and 32k 
> counter")
> 
> The timers are now implemented in drivers/clocksource/timer-ti-dm.c and
> drivers/clocksource/timer-ti-dm-systimer.c. The function
> dmtimer_clocksource_suspend() disables the dmtimer and clock but does
> not ack any interrupts.
> 
> Tony suggested the right place to ack the interrupt during suspend is
> in CPU_CLUSTER_PM_ENTER inside omap_timer_context_notifier().
> 
> Do you think that would be an acceptable approach?

The real issue is that you are apparently suspending from within an
interrupt handler. This is what should be addressed.

Please don´t randomly call into the irqchip code. It will eventually
break, and sooner rather than later.

         M.
-- 
Jazz is not dead. It just smells funny...