From patchwork Thu Jun 4 23:01:46 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Hilman X-Patchwork-Id: 28045 X-Patchwork-Delegate: khilman@deeprootsystems.com Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n54N1o0e022719 for ; Thu, 4 Jun 2009 23:01:51 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751331AbZFDXBr (ORCPT ); Thu, 4 Jun 2009 19:01:47 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751753AbZFDXBr (ORCPT ); Thu, 4 Jun 2009 19:01:47 -0400 Received: from mail-pz0-f171.google.com ([209.85.222.171]:65050 "EHLO mail-pz0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751331AbZFDXBq (ORCPT ); Thu, 4 Jun 2009 19:01:46 -0400 Received: by pzk1 with SMTP id 1so462945pzk.33 for ; Thu, 04 Jun 2009 16:01:48 -0700 (PDT) Received: by 10.143.158.2 with SMTP id k2mr1257131wfo.245.1244156508659; Thu, 04 Jun 2009 16:01:48 -0700 (PDT) Received: from localhost ([216.254.16.51]) by mx.google.com with ESMTPS id 30sm2600693wfa.15.2009.06.04.16.01.47 (version=TLSv1/SSLv3 cipher=RC4-MD5); Thu, 04 Jun 2009 16:01:48 -0700 (PDT) To: "Wang Sawsd-A24013" Cc: , , "Mike Chan" Subject: Re: [PATCH] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable References: From: Kevin Hilman Organization: Deep Root Systems, LLC Date: Thu, 04 Jun 2009 16:01:46 -0700 In-Reply-To: (Wang Sawsd-A's message of "Fri\, 5 Jun 2009 05\:58\:18 +0800") Message-ID: <87d49jeh7p.fsf@deeprootsystems.com> User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux) MIME-Version: 1.0 Sender: linux-omap-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-omap@vger.kernel.org "Wang Sawsd-A24013" writes: > >> What do you think about disabling the level/edge detection when >> disable_irq_wake() is called instead? This seems more logical >> and expected. > > Kevin, if we look at the current code, enable_irq_wake and > disable_irq_wake Does not even touch any GPIO WAKEEN register, it > seems it is intended To just log the gpio bit and enable its WAKEUP > and IOPAD wakeup when suspend happens. Correct. > And also, enable_irq_wake/disable_irq_wake > Are designed to be able used when both IRQ is enabled AND disabled, > In another words, enable_irq_wake may be called after irq_disable, > Disable_irq_wake may be called after irq_enable, if we change > Level/edge detect then it may cause either IRQ never happen Good point. > After irq_enable, or IRQ staus bit also set after irq_disable. Since > The root reason is the level/edge detect can cause IRQ status, it > Is related with IRQ, not wakeup. Correct again. > What do you think? I'm thinking I'm not thinking very clearly on the subject today. It's too hot in Seattle today. ;) I'm also thinking that this isn't just going to be a problem with suspend/resume but also for hitting retention in idle. Any level-triggered GPIO IRQ that is masked, yet still has level/edge detect configured can prevent retention during idle since it will cause IRQ status as you've pointed out. Can you think of any reason not to disable the level/edge detect in the ->mask() hook and to re-enable it in the ->unmask hook? Something like the patch below? Could you try this patch with your TS GPIO configured as level-triggered? Kevin commit f8eb69a2edd684c9e0b72bc3c84c6af9718bd4a4 Author: Kevin Hilman Date: Thu Jun 4 15:57:10 2009 -0700 OMAP: GPIO: clear/restore level/edge detect settings on mask/unmask Signed-off-by: Kevin Hilman --- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c index 3b2054b..83ac494 100644 --- a/arch/arm/plat-omap/gpio.c +++ b/arch/arm/plat-omap/gpio.c @@ -1135,6 +1135,7 @@ static void gpio_mask_irq(unsigned int irq) struct gpio_bank *bank = get_irq_chip_data(irq); _set_gpio_irqenable(bank, gpio, 0); + _set_gpio_triggering(bank, get_gpio_index(gpio), IRQ_TYPE_NONE); } static void gpio_unmask_irq(unsigned int irq) @@ -1142,6 +1143,11 @@ static void gpio_unmask_irq(unsigned int irq) unsigned int gpio = irq - IH_GPIO_BASE; struct gpio_bank *bank = get_irq_chip_data(irq); unsigned int irq_mask = 1 << get_gpio_index(gpio); + struct irq_desc *desc = irq_to_desc(irq); + u32 trigger = desc->status & IRQ_TYPE_SENSE_MASK; + + if (trigger) + _set_gpio_triggering(bank, get_gpio_index(gpio), trigger); /* For level-triggered GPIOs, the clearing must be done after * the HW source is cleared, thus after the handler has run */