Messages in this thread | | | Subject | Re: [PATCH] clocksource/drivers/timer-mediatek: optimize systimer irq clear flow on Mediatek Socs | From | Fengquan Chen <> | Date | Wed, 21 Apr 2021 18:07:52 +0800 |
| |
On Mon, 2021-03-22 at 11:10 +0100, Daniel Lezcano wrote: > 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.
Thanks for review, has been fixed in V4: https://patchwork.kernel.org/project/linux-mediatek/patch/1617960162-1988-2-git-send-email-Fengquan.Chen@mediatek.com/
> > > 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) > > { > > /* 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. > */ >
Thanks for review, descriptions about SYST_CON_IRQ_CLR/SYST_CON_EN/SYST_CON_IRQ_EN have been updated in v4.
And for systimer, there's a hw limitation that before clearing pending irqs, we must enable timer first. so we added a SYST_CON_EN write before SYST_CON_IRQ_CLR write to ensure irq clear successfully.
Also, we cannot only write SYST_CON_IRQ_CLR without SYST_CON_EN bit, because EN bit is also timer clock enable bit.
this is a hardward design limitation but not a bug.
> > > 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))); > > > > > >
| |