From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C2C1EECDE43 for ; Mon, 22 Oct 2018 09:27:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 94EFC20658 for ; Mon, 22 Oct 2018 09:27:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 94EFC20658 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728276AbeJVRon (ORCPT ); Mon, 22 Oct 2018 13:44:43 -0400 Received: from foss.arm.com ([217.140.101.70]:43830 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727509AbeJVRon (ORCPT ); Mon, 22 Oct 2018 13:44:43 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 758D9EBD; Mon, 22 Oct 2018 02:27:01 -0700 (PDT) Received: from big-swifty.misterjones.org (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id EC29B3F6A8; Mon, 22 Oct 2018 02:26:58 -0700 (PDT) Date: Mon, 22 Oct 2018 10:26:53 +0100 Message-ID: <86a7n6tjqa.wl-marc.zyngier@arm.com> From: Marc Zyngier To: Lina Iyer Cc: sboyd@kernel.org, evgreen@chromium.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO In-Reply-To: <20181019194712.GB17444@codeaurora.org> References: <20181011002958.2597-1-ilina@codeaurora.org> <20181011002958.2597-2-ilina@codeaurora.org> <20181019153222.GA17444@codeaurora.org> <20181019194712.GB17444@codeaurora.org> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/25.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Organization: ARM Ltd MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 19 Oct 2018 20:47:12 +0100, Lina Iyer wrote: > > On Fri, Oct 19 2018 at 09:53 -0600, Marc Zyngier wrote: > > Hi Lina, > > > > On 19/10/18 16:32, Lina Iyer wrote: > >> Hi folks, > >> > >> On Wed, Oct 10 2018 at 18:30 -0600, Lina Iyer wrote: > > [...] > > >>> +static irqreturn_t wake_irq_gpio_handler(int irq, void *data) > >>> +{ > >>> + struct irq_data *irqd = data; > >>> + struct irq_desc *desc = irq_to_desc(irqd->irq); > >>> + > >>> + desc->handle_irq(desc); > >> Do we see any problem calling handle_irq()? > > > > Good timing, I was just looking at this. > > > :) Thanks for your time. > > > One thing I can see is that you will end-up calling the EOI callback on > > the root interrupt controller (the GIC), thus writing to ICC_EOIR1_EL1. > > > > But you've never acked this interrupt the first place by reading > > ICC_IAR1_EL1, and that puts you violently out of spec, according to the > > GICv3 spec (8.2.10), which reads: > > > > "A write to this register must correspond to the most recent valid read > > by this PE from an Interrupt Acknowledge Register, and must correspond > > to the INTID that was read from ICC_IAR1_EL1, otherwise the system > > behavior is UNPREDICTABLE." > > > > Here, you definitely risk the sanity of the CPU interface state machine. > > > Oh, thanks Marc. Will look into it. The problem is because I call > handle_irq() directly for the GPIO IRQ which is not triggered but we end > up mask, eoi etc. > > How about calling handle_simple_irq(), which doesn't seem to the IRQ > registers? The problem is that you cannot decide to use another flow handler, as this handler is a constraint imposed by the root interrupt controller. You can overload it, but you then need to make sure that the interrupt will *never* fire at the GIC level, ever. Can you actually enforce this? Assuming you can, this could work. But then the subsequent question is: Why do you have the interrupt at the TLMM level at all? Overall, I'm a bit worried of this "now you see me, now you don't" kind of game behind the kernel back. Is there a way we can stop playing that game and stick to one single path for interrupt delivery? > > So stepping back a bit: At some point, you had a version that just > > relied on regenerating edge interrupts by writing to some register > > (knowing that level interrupts are safe by definition). Why isn't that > > the right solution? It'd avoid the above minefield by just letting the > > HW do its thing... > > > There are some unnecessary complexity with the approach that we are > trying to avoid. > > The TLMM may or not may not be powered off (depending on the SoC state) > and Linux has no control on it. The PDC will remain powered on but we > don't want the PDC interrupt enabled always, since we will receive to > interrupts (one from TLMM and one from PDC) for every event. So we have > to keep the PDC interrupt configured as wakeup interrupt but operate on > the fact that when we go into suspend or cpuidle we will have to switch > to enabling the PDC interrupt and disabling the GPIO IRQ and swap back > when we resume. This dance is harder with cpuidle (notifying the TLMM > driver, when all the CPUs are in idle) than suspend/resume which has > nice callbacks and is what we are trying to avoid but using the PDC > interrupt always. It looks to me that the way this logic is split across two drivers is a major cause of headache. My advise is that either you have one single point of interrupt handling for such interrupt, or you force a TLMM wake-up on such an event, forcing it to handle the interrupt the "normal way"... Thanks, M. -- Jazz is not dead, it just smell funny.