Re: [PATCH 2/2] drivers/clocksource/mediatek: Ack and disable interrupts on shutdown

From: Evan Benn <evanbenn@chromium.org>
To: LKML <linux-kernel@vger.kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Yingjoe Chen <yingjoe.chen@mediatek.com>,
	Julia Lawall <Julia.Lawall@lip6.fr>,
	"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
	<linux-arm-kernel@lists.infradead.org>,
	Stanley Chu <stanley.chu@mediatek.com>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	Fabien Parent <fparent@baylibre.com>,
	"moderated list:ARM/Mediatek SoC support" 
	<linux-mediatek@lists.infradead.org>
Subject: Re: [PATCH 2/2] drivers/clocksource/mediatek: Ack and disable interrupts on shutdown
Date: Fri, 19 Mar 2021 10:46:55 +1100	[thread overview]
Message-ID: <CAKz_xw1BYap=igtp0V8Wdo6vW+3=jvsVDJtukmMoPhvDVeMpRA@mail.gmail.com> (raw)
In-Reply-To: <20210318160414.2.I1d9917047de06715da16e1620759f703fcfdcbcb@changeid>

Hello,

There is a suspend failure on mt8173 chromebooks that use this timer.
The failure shows as an errno: -95 failure with none device.

I tracked this down to the arm trusted firmware aborting the suspend
due to this timer having a pending IRQ, due to not being disabled
during suspend / clockevents_shutdown.

Also reviewed here vs the 4.19 chromeos kernel branch:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2766504

Thanks

Evan Benn

On Thu, Mar 18, 2021 at 4:05 PM Evan Benn <evanbenn@chromium.org> wrote:
>
> set_state_shutdown is called during system suspend after interrupts have
> been disabled. If the timer has fired in the meantime, there will be
> a pending IRQ. So we ack that now and disable the timer. Without this
> ARM trusted firmware will abort the suspend due to the pending
> interrupt.
>
> Now always disable the IRQ in state transitions, and re-enable in
> set_periodic and next_event.
>
> Signed-off-by: Evan Benn <evanbenn@chromium.org>
> ---
>
>  drivers/clocksource/timer-mediatek-mt6577.c | 49 +++++++++++++--------
>  1 file changed, 30 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/clocksource/timer-mediatek-mt6577.c b/drivers/clocksource/timer-mediatek-mt6577.c
> index 9e5241d1876d..44598121585c 100644
> --- a/drivers/clocksource/timer-mediatek-mt6577.c
> +++ b/drivers/clocksource/timer-mediatek-mt6577.c
> @@ -54,13 +54,33 @@ static u64 notrace mtk_gpt_read_sched_clock(void)
>         return readl_relaxed(gpt_sched_reg);
>  }
>
> +static void mtk_gpt_disable_ack_interrupts(struct timer_of *to, u8 timer)
> +{
> +       u32 val;
> +
> +       /* Disable interrupts */
> +       val = readl(timer_of_base(to) + GPT_IRQ_EN_REG);
> +       writel(val & ~GPT_IRQ_ENABLE(timer), timer_of_base(to) +
> +              GPT_IRQ_EN_REG);
> +
> +       /* Ack interrupts */
> +       writel(GPT_IRQ_ACK(timer), timer_of_base(to) + GPT_IRQ_ACK_REG);
> +}
> +
>  static void mtk_gpt_clkevt_time_stop(struct timer_of *to, u8 timer)
>  {
>         u32 val;
>
> +       /* Disable timer */
>         val = readl(timer_of_base(to) + GPT_CTRL_REG(timer));
>         writel(val & ~GPT_CTRL_ENABLE, timer_of_base(to) +
>                GPT_CTRL_REG(timer));
> +
> +       /* This may be called with interrupts disabled,
> +        * so we need to ack any interrupt that is pending
> +        * Or for example ATF will prevent a suspend from completing.
> +        */
> +       mtk_gpt_disable_ack_interrupts(to, timer);
>  }
>
>  static void mtk_gpt_clkevt_time_setup(struct timer_of *to,
> @@ -74,8 +94,10 @@ static void mtk_gpt_clkevt_time_start(struct timer_of *to,
>  {
>         u32 val;
>
> -       /* Acknowledge interrupt */
> -       writel(GPT_IRQ_ACK(timer), timer_of_base(to) + GPT_IRQ_ACK_REG);
> +       /* Enable interrupts */
> +       val = readl(timer_of_base(to) + GPT_IRQ_EN_REG);
> +       writel(val | GPT_IRQ_ENABLE(timer),
> +              timer_of_base(to) + GPT_IRQ_EN_REG);
>
>         val = readl(timer_of_base(to) + GPT_CTRL_REG(timer));
>
> @@ -148,21 +170,6 @@ __init mtk_gpt_setup(struct timer_of *to, u8 timer, u8 option)
>                timer_of_base(to) + GPT_CTRL_REG(timer));
>  }
>
> -static void mtk_gpt_enable_irq(struct timer_of *to, u8 timer)
> -{
> -       u32 val;
> -
> -       /* Disable all interrupts */
> -       writel(0x0, timer_of_base(to) + GPT_IRQ_EN_REG);
> -
> -       /* Acknowledge all spurious pending interrupts */
> -       writel(0x3f, timer_of_base(to) + GPT_IRQ_ACK_REG);
> -
> -       val = readl(timer_of_base(to) + GPT_IRQ_EN_REG);
> -       writel(val | GPT_IRQ_ENABLE(timer),
> -              timer_of_base(to) + GPT_IRQ_EN_REG);
> -}
> -
>  static struct timer_of to = {
>         .flags = TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK,
>
> @@ -193,6 +200,12 @@ static int __init mtk_gpt_init(struct device_node *node)
>         if (ret)
>                 return ret;
>
> +       /* In case the firmware left the interrupts enabled
> +        * disable and ack those now
> +        */
> +       mtk_gpt_disable_ack_interrupts(&to, TIMER_CLK_SRC);
> +       mtk_gpt_disable_ack_interrupts(&to, TIMER_CLK_EVT);
> +
>         /* Configure clock source */
>         mtk_gpt_setup(&to, TIMER_CLK_SRC, GPT_CTRL_OP_FREERUN);
>         clocksource_mmio_init(timer_of_base(&to) + GPT_CNT_REG(TIMER_CLK_SRC),
> @@ -206,8 +219,6 @@ static int __init mtk_gpt_init(struct device_node *node)
>         clockevents_config_and_register(&to.clkevt, timer_of_rate(&to),
>                                         TIMER_SYNC_TICKS, 0xffffffff);
>
> -       mtk_gpt_enable_irq(&to, TIMER_CLK_EVT);
> -
>         return 0;
>  }
>  TIMER_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_gpt_init);
> --
> 2.31.0.rc2.261.g7f71774620-goog
>