From patchwork Thu Sep 18 07:24:15 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chanho Min X-Patchwork-Id: 4929541 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 76A8B9F2EC for ; Thu, 18 Sep 2014 07:26:39 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id B4A8C201B9 for ; Thu, 18 Sep 2014 07:27:28 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 89E782017D for ; Thu, 18 Sep 2014 07:27:27 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1XUW58-0006I5-HZ; Thu, 18 Sep 2014 07:24:46 +0000 Received: from lgeamrelo01.lge.com ([156.147.1.125]) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1XUW55-0006GR-9n for linux-arm-kernel@lists.infradead.org; Thu, 18 Sep 2014 07:24:44 +0000 Received: from unknown (HELO chanhomin01) (10.186.168.102) by 156.147.1.125 with ESMTP; 18 Sep 2014 16:24:15 +0900 X-Original-SENDERIP: 10.186.168.102 X-Original-MAILFROM: chanho.min@lge.com From: "Chanho Min" To: "'Russell King - ARM Linux'" References: <1410953686-15072-1-git-send-email-chanho.min@lge.com> <20140917115758.GQ12361@n2100.arm.linux.org.uk> In-Reply-To: <20140917115758.GQ12361@n2100.arm.linux.org.uk> Subject: RE: [PATCH v2] ARM: timer-sp: ensure interrupt is cleared at sp804_of_init Date: Thu, 18 Sep 2014 16:24:15 +0900 Message-ID: <002c01cfd311$8d3e0330$a7ba0990$@min@lge.com> MIME-Version: 1.0 X-Mailer: Microsoft Office Outlook 12.0 Thread-Index: Ac/SbqX7jhSBk+sbSfuP4TLS4ohlmgAnl0vg Content-Language: ko X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20140918_002443_806330_EDFF6A4C X-CRM114-Status: GOOD ( 24.82 ) X-Spam-Score: -2.2 (--) Cc: 'Michael Opdenacker' , 'Jongsung Kim' , 'Linus Walleij' , 'Stephen Boyd' , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-1.6 required=5.0 tests=BAYES_00, MSGID_MULTIPLE_AT, RCVD_IN_DNSWL_NONE,RP_MATCHES_RCVD,UNPARSEABLE_RELAY autolearn=no version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP > -----Original Message----- > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk] > Sent: Wednesday, September 17, 2014 8:58 PM > To: Chanho Min > Cc: Stephen Boyd; Michael Opdenacker; Linus Walleij; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; Jongsung Kim > Subject: Re: [PATCH v2] ARM: timer-sp: ensure interrupt is cleared at sp804_of_init > > On Wed, Sep 17, 2014 at 08:34:46PM +0900, Chanho Min wrote: > > sp804 may not be added to the tick device if the higher device is > > already registered. In this case, If pending interrupt is existed > > (usually It will be passed from the boot loader), inetrrupt is occured > > without event_handler then it cause kernel panic. So Interrupts > > should be cleared before clockevent is registered. > > > > Changes since v1: > > - Move to sp804_of_init > > - Clear TIMER2 interrupt > > - Update commit log > > > > Signed-off-by: Chanho Min > > --- > > arch/arm/common/timer-sp.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/arch/arm/common/timer-sp.c b/arch/arm/common/timer-sp.c > > index fd6bff0..e3cc08e 100644 > > --- a/arch/arm/common/timer-sp.c > > +++ b/arch/arm/common/timer-sp.c > > @@ -226,6 +226,10 @@ static void __init sp804_of_init(struct device_node *np) > > writel(0, base + TIMER_CTRL); > > writel(0, base + TIMER_2_BASE + TIMER_CTRL); > > > > + /* Ensure interrupt is cleared */ > > + writel(1, base + TIMER_INTCLR); > > + writel(1, base + TIMER_2_BASE + TIMER_INTCLR); > > NAK. > > This is really not necessary for two reasons, and incorrect for a third > reason: > > 1. If the control register is cleared, interrupts are disabled. When > interrupts are disabled, the IRQ line from the timer module is > deasserted irrespective of the internal interrupt state of the timer. > Even if the control register is cleared and interrupts are disabled, It is not harmful before timer is enabled but It is rather harmful if interrupt is asserted before timer up. My patch just ensure interrupt is cleared before timer up > 2. We only enable the interrupt when we set the timer up to run in either > periodic or one-shot modes. If the timer is not used, the interrupt > remains masked. > In our test, setup_irq enables the interrupt irrespective of the timer up/down. > 3. Even if this was necessary (which it isn't), only doing this in the > sp804_of_init() path is wrong - there are other initialisation paths > in this code, and there's no reason why one should have a different > behaviour to the others. So Can another approach be acceptable as bellows? > If you've found this by running the kernel with QEMU, then it's probably > a QEMU bug if it raises an interrupt during the above code. > > -- > FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up > according to speedtest.net. Chaho diff --git a/arch/arm/common/timer-sp.c b/arch/arm/common/timer-sp.c index fd6bff0..4aed813 100644 --- a/arch/arm/common/timer-sp.c +++ b/arch/arm/common/timer-sp.c @@ -121,8 +121,8 @@ static irqreturn_t sp804_timer_interrupt(int irq, void *dev_id) /* clear the interrupt */ writel(1, clkevt_base + TIMER_INTCLR); - - evt->event_handler(evt); + if(evt->event_handler) + evt->event_handler(evt); return IRQ_HANDLED; }