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=-7.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 51E98C433FE for ; Thu, 3 Dec 2020 21:05:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DABE0207AF for ; Thu, 3 Dec 2020 21:05:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727978AbgLCVFR (ORCPT ); Thu, 3 Dec 2020 16:05:17 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45176 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726727AbgLCVFQ (ORCPT ); Thu, 3 Dec 2020 16:05:16 -0500 Received: from mail-vk1-xa44.google.com (mail-vk1-xa44.google.com [IPv6:2607:f8b0:4864:20::a44]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 52780C061A4F for ; Thu, 3 Dec 2020 13:04:36 -0800 (PST) Received: by mail-vk1-xa44.google.com with SMTP id v5so747796vkn.12 for ; Thu, 03 Dec 2020 13:04:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=f9FfLi8Nl7FF4traG898yrie9ssHxyCW2fqoUTn0pwI=; b=HD8E+QIOXUbv5qxbihazg3iQqaBW4ARO8iibzqKW/locEieSrh6fFZxam81Rd21TVA 6vii6LoQG8DDaDQVasrDD3sg4epdc7bJ12hTQMktaajIlNo/+W/pdDw6CL5FN7nNhXMI NHWHMhVR+ZUrXeeOiAme3OlvBBOWRJbmI0yyA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=f9FfLi8Nl7FF4traG898yrie9ssHxyCW2fqoUTn0pwI=; b=jsXmAfWBG0f0WJEblwyxtdzuSad+AHMo/khXhxy6Db87Q7VxFsMR3In1Xsl9EZZCCj HBKEO0hsfxleMgpueVSTnUXD4IrtMHXwzgLQGv5w7RCPsomZxzwQ6h1TJO95iAm3NjJr vL3/IOFBhlW4oTw8HseReH4HvpT+6f376GLbOY0XSTgfD2+CWqVT1LCqS4dcsI7P31gd +xWrQ3YR665Ts5J74LnzkpsmtoD5QySPREsHwOY4gHXUzv37CF26yGI1ub4rllFXhV3T b/q8guRqMGlb5qFclpuE0J8j4wCSradw44TtNClh+82qcIkjGobo5CSP+cTysrrkSRyX d9pw== X-Gm-Message-State: AOAM530jnQJFbvYHciaxs5mYBDQaAlSU1w1bIn2r8LHTzp15WudQQ2rn iiR6KdLmoivcD+oXmnB8eu0C7X+qSwtCQg== X-Google-Smtp-Source: ABdhPJxTLq3+zYrjvHyWl3Cw7sUKLGx62tQQ0MrQbw+FA3ksJYbcz1pwnQRxoewz6PkuMgrEwT+r8w== X-Received: by 2002:a1f:aac6:: with SMTP id t189mr1222740vke.4.1607029474760; Thu, 03 Dec 2020 13:04:34 -0800 (PST) Received: from mail-ua1-f51.google.com (mail-ua1-f51.google.com. [209.85.222.51]) by smtp.gmail.com with ESMTPSA id k207sm102321vkk.15.2020.12.03.13.04.33 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 03 Dec 2020 13:04:33 -0800 (PST) Received: by mail-ua1-f51.google.com with SMTP id r23so1160423uak.0 for ; Thu, 03 Dec 2020 13:04:33 -0800 (PST) X-Received: by 2002:ab0:6285:: with SMTP id z5mr964298uao.0.1607029473177; Thu, 03 Dec 2020 13:04:33 -0800 (PST) MIME-Version: 1.0 References: <20201124094636.v2.1.I2702919afc253e2a451bebc3b701b462b2d22344@changeid> <20201124094636.v2.3.I771b6594b2a4d5b7fe7e12a991a6640f46386e8d@changeid> In-Reply-To: From: Doug Anderson Date: Thu, 3 Dec 2020 13:04:21 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2 3/3] pinctrl: qcom: Clear possible pending irq when remuxing GPIOs To: Maulik Shah Cc: Rajendra Nayak , Marc Zyngier , Thomas Gleixner , Jason Cooper , Linus Walleij , "open list:GPIO SUBSYSTEM" , Neeraj Upadhyay , Stephen Boyd , Bjorn Andersson , Srinivas Ramana , linux-arm-msm , Andy Gross , LKML Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Thu, Dec 3, 2020 at 3:22 AM Maulik Shah wrote: > > >>> + /* > >>> + * Clear IRQs if switching to/from GPIO mode since muxing to/from > >>> + * the GPIO path can cause phantom edges. > >>> + */ > >>> + old_i = (oldval & mask) >> g->mux_bit; > >>> + if (old_i != i && > >>> + (i == pctrl->soc->gpio_func || old_i == pctrl->soc->gpio_func)) > >>> + msm_pinctrl_clear_pending_irq(pctrl, group, irq); > >>> + > >> The phantom irq can come when switching to GPIO irq mode. so may be only > >> check if (i == pctrl->soc->gpio_func) { > > Have you tested this experimentally? > Yes Yes means that you tried switching away from GPIO mode and you couldn't get a phantom interrupt? OK, I'll re-test then. I'll test on the Chrome OS kernel tree since that's easiest for me, but I can test on mainline if you think it would make a difference... 1. Pick and put that kernel on the device. 2. In Cr50 console, make the WP line low with: wp enable 3. In AP console do: echo bogus > /sys/module/gpio_keys/parameters/doug_test 4. See bogus interrupt: localhost ~ # echo bogus > /sys/module/gpio_keys/parameters/doug_test [ 62.006346] DOUG: selecting state bogus [ 62.011813] DOUG: ret 0 [ 62.011875] DOUG: in dual edge parent: hwirq=66, type=1 [ 62.020300] DOUG: gpio_keys_gpio_isr Can you try replicating again? > > I have experimentally tested this and I can actually see an interrupt > > generated when I _leave_ GPIO as well as when I enter GPIO mode. If > > you can't see this I can re-setup my test, but this was one of those > > things that convinced me that the _transition_ is what was causing the > > fake interrupt. > > > > I think my test CL can help you with > > testing if you wish. > > > > > >> even better if you can clear this unconditionally. > > Why? It should only matter if we're going to/from GPIO mode. > > Probably i was not clear, the phantom irq should be cleared when > switching gpio to gpio IRQ mode. > > When GPIO was used as Rx line in example QUP/UART use case, it can latch > the phantom IRQ This is where I disagree with you. I don't think the interrupt is latching while it's used as an Rx line. I think it's the pinmux change that introduces an phantom interrupt. Specifically, with the same test patch above, AKA , I can do this: 1. On AP: echo bogus > /sys/module/gpio_keys/parameters/doug_test 2. On Cr50 console: wp disable wp enable wp disable wp enable wp disable wp enable 3. Go back and check the AP and see that no interrupts fired. Said another way: when we're muxed away the interrupts aren't getting latched. It's the act of changing the mux that causes the phantom interrupts. > but as long as its IRQ is in disabled/masked state it > doesn't matter. ...but there's no requirement that someone would need to disable/mask an interrupt while switching the muxing, is there? So it does matter. > its only when the GPIO is again set to IRQ mode with set_mux callback, > the phantom IRQ needs clear to start as clean. > > So we should check only for if (i == pctrl->soc->gpio_func) then clear > phantom IRQ. > > The same is case with .direction_output callback, when GPIO is used as > output say as clock, need not clear any phantom IRQ, > > The reason is with every pulse of clock it can latch as pending IRQ in > GIC_ISPEND as long as it stay as output mode/clock. > > its only when switching back GPIO from output direction to input & IRQ > function, need to clear the phantom IRQ. > > so we do not require clear phantom irq in .direction_output callback. I think all the above explanation is with the model that the interrupt detection logic is still happening even when muxed away. I don't believe that's true. Please run my test patch or code up something similar yourself. > >> In step (3) msm_gpio_irq_set_type() touches the RAW_STATUS_EN making the > >> phantom irq pending again. > >> To resolve this, you will need to invoke msm_pinctrl_clear_pending_irq() > >> at the end of the msm_gpio_irq_set_type(). > >> > >> I would like Rajendra's (already in cc) review as well on above part. > > Ugh, so we need a clear in yet another place. Joy. OK, I will wait > > for Rajendra's comment but I can add similar code in > > msm_gpio_irq_enable(). > > As the clearing phantom irq code in msm_gpio_irq_enable() is moved to > separate function msm_pinctrl_clear_pending_irq(), it needs invoke from > at the end of msm_gpio_irq_set_type() too. Seems reasonable to me. I'll include this in my next spin. Still waiting for us to agree on some of the points above before spinning, though. -Doug