* [PATCH] clocksource/drivers/timer-mediatek: optimize systimer irq clear flow on Mediatek Socs @ 2021-03-02 7:28 Fengquan Chen 2021-03-02 7:28 ` Fengquan Chen 0 siblings, 1 reply; 6+ messages in thread From: Fengquan Chen @ 2021-03-02 7:28 UTC (permalink / raw) To: Daniel Lezcano, Thomas Gleixner, Matthias Brugger, linux-kernel, linux-arm-kernel, linux-mediatek Cc: Fengquan Chen, dehui.sun 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@mediatek.com> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] clocksource/drivers/timer-mediatek: optimize systimer irq clear flow on Mediatek Socs 2021-03-02 7:28 [PATCH] clocksource/drivers/timer-mediatek: optimize systimer irq clear flow on Mediatek Socs Fengquan Chen @ 2021-03-02 7:28 ` Fengquan Chen 2021-03-22 10:10 ` Daniel Lezcano 2021-03-23 0:48 ` Evan Benn 0 siblings, 2 replies; 6+ messages in thread From: Fengquan Chen @ 2021-03-02 7:28 UTC (permalink / raw) To: Daniel Lezcano, Thomas Gleixner, Matthias Brugger, linux-kernel, linux-arm-kernel, linux-mediatek Cc: Fengquan Chen, dehui.sun 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 <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)); 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] clocksource/drivers/timer-mediatek: optimize systimer irq clear flow on Mediatek Socs 2021-03-02 7:28 ` Fengquan Chen @ 2021-03-22 10:10 ` Daniel Lezcano 2021-04-21 10:07 ` Fengquan Chen 2021-03-23 0:48 ` Evan Benn 1 sibling, 1 reply; 6+ messages in thread From: Daniel Lezcano @ 2021-03-22 10:10 UTC (permalink / raw) To: Fengquan Chen, Thomas Gleixner, Matthias Brugger, linux-kernel, linux-arm-kernel, linux-mediatek Cc: dehui.sun 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 <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. */ > 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))); > > -- <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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] clocksource/drivers/timer-mediatek: optimize systimer irq clear flow on Mediatek Socs 2021-03-22 10:10 ` Daniel Lezcano @ 2021-04-21 10:07 ` Fengquan Chen 0 siblings, 0 replies; 6+ messages in thread From: Fengquan Chen @ 2021-04-21 10:07 UTC (permalink / raw) To: Daniel Lezcano, Thomas Gleixner, Matthias Brugger, dehui.sun Cc: linux-kernel, linux-arm-kernel, linux-mediatek 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))); > > > > > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] clocksource/drivers/timer-mediatek: optimize systimer irq clear flow on Mediatek Socs 2021-03-02 7:28 ` Fengquan Chen 2021-03-22 10:10 ` Daniel Lezcano @ 2021-03-23 0:48 ` Evan Benn 2021-04-21 10:17 ` Fengquan Chen 1 sibling, 1 reply; 6+ messages in thread From: Evan Benn @ 2021-03-23 0:48 UTC (permalink / raw) To: Fengquan Chen Cc: Daniel Lezcano, Thomas Gleixner, Matthias Brugger, LKML, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, moderated list:ARM/Mediatek SoC support, dehui.sun 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 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] clocksource/drivers/timer-mediatek: optimize systimer irq clear flow on Mediatek Socs 2021-03-23 0:48 ` Evan Benn @ 2021-04-21 10:17 ` Fengquan Chen 0 siblings, 0 replies; 6+ messages in thread From: Fengquan Chen @ 2021-04-21 10:17 UTC (permalink / raw) To: Evan Benn, Daniel Lezcano, Thomas Gleixner, Matthias Brugger, dehui.sun Cc: LKML, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, moderated list:ARM/Mediatek SoC support On Tue, 2021-03-23 at 11:48 +1100, Evan Benn wrote: > 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. > Thanks for review. For systimer, you must enable timer before clear irq,it's a hw limitation that would be easily neglected. > > > > 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. Thanks for review. it's necessary. As described above,we must enable timer before clear irq, so here is just want to ensure irq clear successfully. We always disable irq here, and will be re-enable in mtk_syst_clkevt_next_event. > > > } > > > > @@ -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? Thanks for review. there's a hw limitation here, we can not clear irq while timer is disabled, and SYST_CON_EN&SYST_CON_IRQ_CLR bit must be write at the same time or can not write SYST_CON_IRQ_CLR bit seperately. > > > > > -- > > 1.8.1.1.dirty > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-21 10:29 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-02 7:28 [PATCH] clocksource/drivers/timer-mediatek: optimize systimer irq clear flow on Mediatek Socs Fengquan Chen 2021-03-02 7:28 ` Fengquan Chen 2021-03-22 10:10 ` Daniel Lezcano 2021-04-21 10:07 ` Fengquan Chen 2021-03-23 0:48 ` Evan Benn 2021-04-21 10:17 ` Fengquan Chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).