Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752555AbbHTO2b (ORCPT ); Thu, 20 Aug 2015 10:28:31 -0400 Received: from mail-pa0-f48.google.com ([209.85.220.48]:36624 "EHLO mail-pa0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751284AbbHTO22 (ORCPT ); Thu, 20 Aug 2015 10:28:28 -0400 Message-ID: <55D5E40A.6080904@linaro.org> Date: Thu, 20 Aug 2015 16:28:26 +0200 From: Daniel Lezcano User-Agent: Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 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" Subject: Re: [PATCH v2 1/5] clocksource: mediatek: do not enable GPT_CLK_EVT when setup References: <1437552878-3684-1-git-send-email-yingjoe.chen@mediatek.com> <55CC56C4.8080201@linaro.org> <1439820602.3414.12.camel@mtksdaap41> In-Reply-To: <1439820602.3414.12.camel@mtksdaap41> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4283 Lines: 121 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 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/001545.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 option) >>> +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 = TIMER_CTRL_OP(option); >>> + if (enable) >>> + val |= TIMER_CTRL_ENABLE; >>> + writel(val, evt->gpt_base + TIMER_CTRL_REG(timer)); >> >> Instead of the 'enable' new option, I prefer a test with 'timer' with a >> comment: >> >> /* >> * the timer hw is broken in that way ... bla bla, so we only >> * enable the clocksource ... >> */ >> if (timer == GPT_CLK_SRC) >> val |= 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.html > http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000551.html I could take or ack this patch trusting it fixes the issue but there are 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 didn't. So how is tested the patch ? Regarding the different fixes for this problem, it sounds like you are proceeding by trial and error. Please give a more detailed analysis of the problem and especially check the timer is not altered by the firmware leaving it in a transient state or whatever. -- Daniel -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/