Re: [PATCH] clocksource/drivers/timer-mediatek: optimize systimer irq clear flow on Mediatek Socs

2021-03-22 Thread Evan Benn 
On Thu, Mar 4, 2021 at 11:07 AM Fengquan Chen
 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 
> ---
>  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
>

Re: [PATCH] clocksource/drivers/timer-mediatek: optimize systimer irq clear flow on Mediatek Socs

2021-03-22 Thread Daniel Lezcano 
On 02/03/2021 08:28, Fengquan Chen wrote:
> 1)ensure systimer is enabled before clear and disable interrupt, which only
> for systimer in Mediatek Socs.
>
> 2)clear any pending timer-irq when shutdown to keep suspend flow clean,
> when use systimer as tick-broadcast timer
> 
> Change-Id: Ia3eda83324af2fdaf5cbb3569a9bf020a11f8009

Remove the above.

Add a Fixes tag.

> Signed-off-by: Fengquan Chen 
> ---
>  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)
>  {
>   /* Clear and disable interrupt */
> + writel(SYST_CON_EN, SYST_CON_REG(to));

SYST_CON_EN is set below, why do you have to do it before?

Is that a hw bug ?

It is confusing what the description of the SYST_CON_EN says:

/*
 * SYST_CON_EN: Clock enable. Shall be set to
 *   - Start timer countdown.
 *   - Allow timeout ticks being updated.
 *   - Allow changing interrupt functions.

What means "interrupt functions" ?

Does writing writel(SYST_CON_EN, SYST_CON_REG(to)) before
SYST_CON_IRQ_CLR allows to clear the interrupt flag?

Can you explain how the timer works regarding this part?

It sounds to me a bit strange.

 *
 * SYST_CON_IRQ_EN: Set to allow interrupt.
 *
 * SYST_CON_IRQ_CLR: Set to clear interrupt.
 */


>   writel(SYST_CON_IRQ_CLR | SYST_CON_EN, SYST_CON_REG(to));
>  }
>  
> @@ -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)));
>  
> 


-- 
 Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

[PATCH] clocksource/drivers/timer-mediatek: optimize systimer irq clear flow on Mediatek Socs

2021-03-02 Thread Fengquan Chen 
clocksource/drivers/timer-mediatek: optimize systimer irq clear flow on 
Mediatek Socs

1)ensure systimer is enabled before clear and disable interrupt, which only
  for systimer in Mediatek Socs.

2)clear any pending timer-irq when shutdown to keep suspend flow clean,
  when use systimer as tick-broadcast timer

Signed-off-by: Fengquan Chen 

Fengquan Chen (1):
  clocksource/drivers/timer-mediatek: optimize systimer irq clear flow
on Mediatek Socs

 drivers/clocksource/timer-mediatek.c | 4 
 1 file changed, 4 insertions(+)

-- 
1.8.1.1.dirty

[PATCH] clocksource/drivers/timer-mediatek: optimize systimer irq clear flow on Mediatek Socs

2021-03-02 Thread Fengquan Chen 
1)ensure systimer is enabled before clear and disable interrupt, which only
for systimer in Mediatek Socs.

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 
---
 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)
 {
/* Clear and disable interrupt */
+   writel(SYST_CON_EN, SYST_CON_REG(to));
writel(SYST_CON_IRQ_CLR | SYST_CON_EN, SYST_CON_REG(to));
 }
 
@@ -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)));
 
-- 
1.8.1.1.dirty