Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1614670085-26229-1-git-send-email-Fengquan.Chen@mediatek.com> <1614670085-26229-2-git-send-email-Fengquan.Chen@mediatek.com>
In-Reply-To: <1614670085-26229-2-git-send-email-Fengquan.Chen@mediatek.com>
From:   Evan Benn <evanbenn@chromium.org>
Date:   Tue, 23 Mar 2021 11:48:52 +1100
Message-ID: <CAKz_xw2abQyS9Vvx7DUppGGd-AaDQAfdAOwi46fB9yLPJWJeUA@mail.gmail.com>
Subject: Re: [PATCH] clocksource/drivers/timer-mediatek: optimize systimer irq
 clear flow on Mediatek Socs
To:     Fengquan Chen <Fengquan.Chen@mediatek.com>
Cc:     Daniel Lezcano <daniel.lezcano@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@lists.infradead.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@lists.infradead.org>, dehui.sun@mediatek.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Mar 4, 2021 at 11:07 AM Fengquan Chen
<Fengquan.Chen@mediatek.com> wrote:
>
> 1)ensure systimer is enabled before clear and disable interrupt, which only
> for systimer in Mediatek Socs.

Why does the timer need to be enabled before the interrupt can be
disabled? The datasheet I have does not suggest that this is required.

>
> 2)clear any pending timer-irq when shutdown to keep suspend flow clean,
> when use systimer as tick-broadcast timer
>
> Change-Id: Ia3eda83324af2fdaf5cbb3569a9bf020a11f8009
> Signed-off-by: Fengquan Chen <fengquan.chen@mediatek.com>
> ---
>  drivers/clocksource/timer-mediatek.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek.c
> index 9318edc..9f1f095dc 100644
> --- a/drivers/clocksource/timer-mediatek.c
> +++ b/drivers/clocksource/timer-mediatek.c
> @@ -75,6 +75,7 @@
>  static void mtk_syst_ack_irq(struct timer_of *to)

This function seems to be mis-named. It does more than just ack the irq.

>  {
>         /* Clear and disable interrupt */
> +       writel(SYST_CON_EN, SYST_CON_REG(to));

This line seems to enable the timer and disable the interrupt.

>         writel(SYST_CON_IRQ_CLR | SYST_CON_EN, SYST_CON_REG(to));

This line acks the interrupt and enables the timer and disables the interrupt.
Are these lines both necessary?
Maybe this function should just ack the interrupt without changing the
other bits.

>  }
>
> @@ -111,6 +112,9 @@ static int mtk_syst_clkevt_next_event(unsigned long ticks,
>
>  static int mtk_syst_clkevt_shutdown(struct clock_event_device *clkevt)
>  {
> +       /* Clear any irq */
> +       mtk_syst_ack_irq(to_timer_of(clkevt));
> +
>         /* Disable timer */
>         writel(0, SYST_CON_REG(to_timer_of(clkevt)));

This is a third write to the same register, I believe all 3 writes can
be combined into 1. Is that possible?

>
> --
> 1.8.1.1.dirty
>