Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752247AbbETH5O (ORCPT ); Wed, 20 May 2015 03:57:14 -0400 Received: from foss.arm.com ([217.140.101.70]:37058 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751530AbbETH5K (ORCPT ); Wed, 20 May 2015 03:57:10 -0400 Date: Wed, 20 May 2015 08:58:46 +0100 From: Marc Zyngier To: Feng Kan Cc: Linus Walleij , Abhijeet Dharmapurikar , Stephen Boyd , Phong Vo , Tin Huynh , Y Vo , Thomas Gleixner , Toan Le , Bjorn Andersson , Jason Cooper , Arnd Bergmann , linux-arm-msm , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-gpio@vger.kernel.org" , Alexandre Courbot Subject: Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state Message-ID: <20150520085846.43ad36b7@why.wild-wind.fr.eu.org> In-Reply-To: References: <1426676484-21812-1-git-send-email-marc.zyngier@arm.com> <1426676484-21812-3-git-send-email-marc.zyngier@arm.com> <20150519110131.522f9c16@arm.com> Organization: ARM Ltd X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.25; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4096 Lines: 96 On Tue, 19 May 2015 22:45:26 +0100 Feng Kan wrote: > On Tue, May 19, 2015 at 3:01 AM, Marc Zyngier wrote: > > On Tue, 19 May 2015 09:40:21 +0100 > > Linus Walleij wrote: > > > >> On Thu, May 14, 2015 at 10:14 PM, Feng Kan wrote: > >> > On Thu, May 14, 2015 at 3:32 AM, Linus Walleij wrote: > >> > >> >> But surely the GPIO block has its own status register, so are > >> >> you saying that this register is unreliable? > >> > > >> > When the GPIO is used as interrupt, the gpio block does not report the > >> > status anymore. Which leaves us stuck with SPISR. > >> >> > >> >> I can think of a few reasons, like transient IRQs etc but > >> >> what is actually causing this? > >> > > >> > I won't say the obvious. > >> > >> Yeah I see your problem now :( > >> > >> I think it's better to fix the access functions so that you can > >> cross-call to the GIC driver to get the SPISR flag out though. > >> Let's see what Marc says. > >> > >> >> Which GPIO driver is this? Is it upstream? > >> > > >> > Yes, it is upstream. It is the xgene slimpro gpio driver. I am starting to > >> > think that we ought to switch to use some gpio poll driver rather than > >> > using gpio-key. > >> > >> There is both gpio_keys_polled and IRQ-driven gpio_keys so yeah > >> that's possible. But honestly I think it's better to deal with this > >> problem for real because IRQ is more efficient. > >> > >> So the way I perceive it this is the real problem: > >> > >> +static int gic_irq_get_irqchip_state(struct irq_data *d, > >> + enum irqchip_irq_state which, bool *val) > >> +{ > >> + switch (which) { > >> (...) > >> + case IRQCHIP_STATE_ACTIVE: > >> + *val = gic_peek_irq(d, GIC_DIST_ACTIVE_SET); > >> + break; case: read > >> from 0xd04 (SPISR) instead, because that makes more > >> sense to me, or am I wrong at it? > >> > >> + case IRQCHIP_STATE_LINE_LEVEL: > >> + *val = gic_peek_irq(d, GIC_DIST_SPISR); > >> + break; > >> > >> And then put a define into for > >> GIC_DIST_SPISR. > > > > What worries me here is that the PENDING state should already give you > > the right level of information (this is what the GIC-400 TRM says). The > > only reason why SPISR exists is that software can write to the PENDING > > register, while SPISR is RO. > > > > If reading the pending state doesn't work, then I'd like to know > > exactly *why*. Only then we can add support for LINE_LEVEL using SPISR > > (which has to be GIC-400 specific, as it is not architected). > > IS_PENDING and IS_ACTIVE works fine for the ISR context. However, > the nature of the register is meant for IRQ handling and not to read > the status of a GPIO. By the time the gpio_key throws a work queue > and check the status of the PENDING register, it is no long relevant. This is far more subtle that that. Assuming a level-triggered interrupt, the PENDING state is completely independent of the state of the interrupt (see the GIC state machine), and will accurately reflect the input line (an edge-triggered interrupt is of course a different matter). As outlined in the GIC-400 TRM, both GICD_ICPENDINGRn, GICD_ISPENDINGRn and GICD_SPISRn are providing the same information (the only difference being that SPISRn is offering a RO view before the latches). SPISRn is essentially a debug register, not something that is meant to be used in normal code, GPIO or otherwise. So assuming we have an actual GIC-400 implementation here, I want to know why this isn't working, and you are in a position to tell me (I assume you can talk to the HW people that did the integration). Thanks, M. -- Without deviation from the norm, progress is not possible. -- 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/