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;
Subject: Re: [PATCH] [v4, 1/1] clocksource/drivers/timer-mediatek: optimize
 systimer irq clear flow on shutdown
To:     Fengquan Chen <Fengquan.Chen@mediatek.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org
Cc:     dehui.sun@mediatek.com, Evan Benn <evanbenn@chromium.org>
References: <1617960162-1988-1-git-send-email-Fengquan.Chen@mediatek.com>
 <1617960162-1988-2-git-send-email-Fengquan.Chen@mediatek.com>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
Message-ID: <4d3b342c-a14f-f4ac-6860-9f6a88a24501@linaro.org>
Date:   Mon, 19 Apr 2021 10:44:34 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <1617960162-1988-2-git-send-email-Fengquan.Chen@mediatek.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 09/04/2021 11:22, Fengquan Chen wrote:
> mtk_syst_clkevt_shutdown is called after irq disabled in suspend flow,
> clear any pending systimer irq when shutdown to avoid suspend aborted
> due to timer irq pending
> 
> Also as for systimer in mediatek socs, there must be firstly enable
> timer before clear systimer irq
> 
> Fixes: e3af677607d9("clocksource/drivers/timer-mediatek: Add support for system timer")
> Signed-off-by: Fengquan Chen <fengquan.chen@mediatek.com>
> 
> ---
>  drivers/clocksource/timer-mediatek.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek.c
> index 9318edc..6461fd3 100644
> --- a/drivers/clocksource/timer-mediatek.c
> +++ b/drivers/clocksource/timer-mediatek.c
> @@ -60,9 +60,9 @@
>   * SYST_CON_EN: Clock enable. Shall be set to
>   *   - Start timer countdown.
>   *   - Allow timeout ticks being updated.
> - *   - Allow changing interrupt functions.
> + *   - Allow changing interrupt status,like clear irq pending.
>   *
> - * SYST_CON_IRQ_EN: Set to allow interrupt.
> + * SYST_CON_IRQ_EN: Set to enable interrupt.
>   *
>   * SYST_CON_IRQ_CLR: Set to clear interrupt.
>   */
> @@ -75,6 +75,7 @@
>  static void mtk_syst_ack_irq(struct timer_of *to)
>  {
>  	/* Clear and disable interrupt */
> +	writel(SYST_CON_EN, SYST_CON_REG(to));
>  	writel(SYST_CON_IRQ_CLR | SYST_CON_EN, SYST_CON_REG(to));
>  }

IIRC, there is a hardware issue here. If it is the case, please describe
it and refer to an errata if any.

Also Evan Benn commented your code and asked a couple of questions [1],
please answer before reposting a new version.

Comments ignored == patch ignored

> @@ -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)));

Please check out the patch sent by Evan Benn [2], if you agree, ack it.

Thanks

  -- Daniel

[1] https://patchwork.kernel.org/comment/24059277/
[2]
https://lore.kernel.org/linux-arm-kernel/20210412132200.v3.1.I1d9917047de06715da16e1620759f703fcfdcbcb@changeid/

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog