From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Lezcano Subject: Re: [PATCH v2 1/5] clocksource: mediatek: do not enable GPT_CLK_EVT when setup Date: Thu, 20 Aug 2015 16:28:26 +0200 Message-ID: <55D5E40A.6080904@linaro.org> References: <1437552878-3684-1-git-send-email-yingjoe.chen@mediatek.com> <55CC56C4.8080201@linaro.org> <1439820602.3414.12.camel@mtksdaap41> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <1439820602.3414.12.camel@mtksdaap41> Sender: linux-kernel-owner@vger.kernel.org To: Yingjoe Chen Cc: Daniel Kurtz , Stephen Boyd , Thomas Gleixner , James Liao , Russell King , srv_heupstream , Arnd Bergmann , "open list:OPEN FIRMWARE AND..." , Catalin Marinas , Michael Turquette , "linux-kernel@vger.kernel.org" , Olof Johansson , Rob Herring , linux-mediatek@lists.infradead.org, Sascha Hauer , Matthias Brugger , linux-clk@vger.kernel.org, "linux-arm-kernel@lists.infradead.org" List-Id: devicetree@vger.kernel.org On 08/17/2015 04:10 PM, Yingjoe Chen wrote: > On Thu, 2015-08-13 at 10:35 +0200, Daniel Lezcano wrote: >> On 07/22/2015 10:14 AM, Yingjoe Chen wrote: >>> Spurious mtk timer interrupt is noticed at boot and cause kernel >>> crash. It seems if GPT is enabled, it will latch irq status even >>> when its IRQ is disabled. It "seems" ? >>> When irq is enabled afterward, we see >>> spurious interrupt. Doesn't have the firmware something to do with that ? I have a mtk 8173 board I can use next week. How do you reproduce the=20 issue ? >>> Change init flow to only enable GPT_CLK_SRC at mtk_timer_init. >>> >>> Acked-by: Matthias Brugger >>> Reviewed-by: Daniel Kurtz >>> Signed-off-by: Yingjoe Chen >>> --- >>> >>> Update to my patch [1], added __init as Daniel suggest. This is the >>> only patch that need to change in that series, so I only sent this = one. >>> >>> http://lists.infradead.org/pipermail/linux-mediatek/2015-July/00154= 5.html >>> >>> drivers/clocksource/mtk_timer.c | 16 ++++++++++------ >>> 1 file changed, 10 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/= mtk_timer.c >>> index 68ab423..2ba5b66 100644 >>> --- a/drivers/clocksource/mtk_timer.c >>> +++ b/drivers/clocksource/mtk_timer.c >>> @@ -156,9 +156,11 @@ static void mtk_timer_global_reset(struct mtk_= clock_event_device *evt) >>> writel(0x3f, evt->gpt_base + GPT_IRQ_ACK_REG); >>> } >>> >>> -static void >>> -mtk_timer_setup(struct mtk_clock_event_device *evt, u8 timer, u8 o= ption) >>> +static void __init mtk_timer_setup(struct mtk_clock_event_device *= evt, >>> + u8 timer, u8 option, bool enable) >>> { >>> + u32 val; >>> + >>> writel(TIMER_CTRL_CLEAR | TIMER_CTRL_DISABLE, >>> evt->gpt_base + TIMER_CTRL_REG(timer)); >>> >>> @@ -167,8 +169,10 @@ mtk_timer_setup(struct mtk_clock_event_device = *evt, u8 timer, u8 option) >>> >>> writel(0x0, evt->gpt_base + TIMER_CMP_REG(timer)); >>> >>> - writel(TIMER_CTRL_OP(option) | TIMER_CTRL_ENABLE, >>> - evt->gpt_base + TIMER_CTRL_REG(timer)); >>> + val =3D TIMER_CTRL_OP(option); >>> + if (enable) >>> + val |=3D TIMER_CTRL_ENABLE; >>> + writel(val, evt->gpt_base + TIMER_CTRL_REG(timer)); >> >> Instead of the 'enable' new option, I prefer a test with 'timer' wit= h a >> comment: >> >> /* >> * the timer hw is broken in that way ... bla bla, so we only >> * enable the clocksource ... >> */ >> if (timer =3D=3D GPT_CLK_SRC) >> val |=3D TIMER_CTRL_ENABLE; > > Hi Daniel, > > Thanks for your review. > Since this bug happens to anyone using interrupt, Can you elaborate ? I don't get the point. > I'm not sure checking > timer and only enable it for GPT_CLK_SRC is easier to read. Anyway, I= 'll > change to this in next version. >> That said, can you have a look at commit 1096be08 ? >> "clockevents: sun5i: Fix setup_irq init sequence" >> >> first and check if moving the interrupt request after the >> clockevents_config_and_register could fix your issue. > > I've tested this before, see: > > http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000539.h= tml > http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000551.h= tml I could take or ack this patch trusting it fixes the issue but there ar= e=20 some points that need clarifications. - Does the spurious interrupt occurs *every* time ? at each boot ? The previous patches were supposed to fix the issue but they actually=20 didn't. So how is tested the patch ? Regarding the different fixes for this problem, it sounds like you are=20 proceeding by trial and error. Please give a more detailed analysis of the problem and especially chec= k=20 the timer is not altered by the firmware leaving it in a transient stat= e=20 or whatever. -- Daniel --=20 Linaro.org =E2=94=82 Open source software fo= r ARM SoCs =46ollow Linaro: Facebook | Twitter | Blog