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=-5.8 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 40371C433FE for ; Thu, 10 Dec 2020 00:46:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id F357222D5B for ; Thu, 10 Dec 2020 00:46:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730747AbgLJAoU (ORCPT ); Wed, 9 Dec 2020 19:44:20 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46794 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730422AbgLJAoG (ORCPT ); Wed, 9 Dec 2020 19:44:06 -0500 Received: from mail-vs1-xe2a.google.com (mail-vs1-xe2a.google.com [IPv6:2607:f8b0:4864:20::e2a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2AAF9C061794 for ; Wed, 9 Dec 2020 16:43:20 -0800 (PST) Received: by mail-vs1-xe2a.google.com with SMTP id s85so1965535vsc.3 for ; Wed, 09 Dec 2020 16:43:20 -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=KQorzTW27QOsMWD3KHq+FAGOyijZWBp7eao6c1glD1g=; b=L2LlsWZYF4pVW8WTWhpUR0IO6rkyLZZ+EBTHXkv0Gmk9+l/0n30L23wN5nChAj5tQQ ZVZxE9/QTq9fGZYLoDk5uWOofg/7bC+O0ACiA1Zgp1k4vENkUvV8yk/kZq/1OpaCgCC1 W4ODAzrUJ/zkEXgLVeyc2BUBA9vh+hEriVW6g= 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=KQorzTW27QOsMWD3KHq+FAGOyijZWBp7eao6c1glD1g=; b=R/0iNAdFTYrLfu5ytFWzIRqj89RQdyFIwPVu8Fck1kXGVG/h/PQ9Iq35qLj1yJNYIq 6YNfnZBYDXcQjGmhhGbbzfSQs920/OaLkOMvg7zjFyfyaf2iEKNeoCzBn7DE/CvW7m+u 72vXX9Wte9W2kHrS4CPUuEPqJY2DstPU/lTBca0jUs8ujWygcYIHZZdiwLhTi1o8dqG7 DFDtAPPy4T5nrsJ0Cr7YWHWfXujr39L8bNX8JOExMSM7Vmp6SRwWTLY+jiCw4UA/dQUK 64oLzx5soGnrCKF28sLF9booD8mY8RKTPJ3rxpz79aP3PxlusaKWeNtpMo/2u7IPNY0L VYkg== X-Gm-Message-State: AOAM5310EZXEhYwXXt4mAF5KUYTfZ2UaLvPSilTG5qe30M7qoECDdpQf SQOYCz0yEAjoJiAjM+vLvBEoX1fmdZCq4w== X-Google-Smtp-Source: ABdhPJyy8g6sI1EY+Okep5+P/S6Ek84oROj6mpSaZHWhZXpjWMJHm6KTKzbW94cQymO3euuJ5jCMCQ== X-Received: by 2002:a05:6102:22fa:: with SMTP id b26mr5141621vsh.35.1607560998712; Wed, 09 Dec 2020 16:43:18 -0800 (PST) Received: from mail-vk1-f177.google.com (mail-vk1-f177.google.com. [209.85.221.177]) by smtp.gmail.com with ESMTPSA id b19sm337507vsq.18.2020.12.09.16.43.17 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 09 Dec 2020 16:43:17 -0800 (PST) Received: by mail-vk1-f177.google.com with SMTP id w190so808263vkg.13 for ; Wed, 09 Dec 2020 16:43:17 -0800 (PST) X-Received: by 2002:a1f:3fc9:: with SMTP id m192mr5293407vka.17.1607560996597; Wed, 09 Dec 2020 16:43:16 -0800 (PST) MIME-Version: 1.0 References: <20201124094636.v2.1.I2702919afc253e2a451bebc3b701b462b2d22344@changeid> <20201124094636.v2.3.I771b6594b2a4d5b7fe7e12a991a6640f46386e8d@changeid> <5f24ec87-6d91-dfd9-0f4f-6687f37c60ac@codeaurora.org> In-Reply-To: <5f24ec87-6d91-dfd9-0f4f-6687f37c60ac@codeaurora.org> From: Doug Anderson Date: Wed, 9 Dec 2020 16:43:05 -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 Tue, Dec 8, 2020 at 9:54 PM Maulik Shah wrote: > > >> 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. > Its not the interrupt detection logic that is still happening when muxed > away, but the GPIO line is routed to GIC from PDC. > The GPIO line get forwarded when the system is active/out of system > level low power mode to GIC irrespective of whether GPIO is used as > interrupt or not. > Due to this it can still latch the IRQ at GIC after switching to lets > say Rx mode, whenever the line has any data recive, the line state > toggles can be latched as error interrupt at GIC. >From my tests, though, I strongly believe that the pin is only visible to the PDC if it's muxed as GPIO. Specifically, in my tests I did this (with all my patches applied so there were no phantom interrupts when remuxing): a) Muxed the pin away from GPIO to special function, but _didn't_ mask the interrupt. b) Toggled the line a whole bunch. These caused no interrupts at all. c) Muxed back to GPIO. To me this is quite strong evidence that the muxing is "earlier" in the path than the connection to the PDC. In other words, if you change the mux away from GPIO then the PDC stops seeing it and thus the GIC also stops seeing it. The GIC can't latch what it can't see. This means while you're in "Rx mode" it can't be latched. OK, so just in case this somehow only happens in S3, I also tried this (with my patch from https://crrev.com/c/2556012): a) Muxed away from GPIO ("bogus" pinmux) b) Enter S3. c) Toggle the GPIO a whole bunch ("wp enable / wp disable" on Cr50). d) Wake from S3. e) Check to see if the interrupt fired a bunch. It didn't fire at all In my test code the interrupt is not masked, only muxed away. That means that if, somehow, the PDC was still observing it then we'd see the interrupt fire. We don't. Unless I messed up in my tests (always possible, though by this point I've run them a number of times) then it still feels like your mental model is wrong, or it's always possible I'm still misunderstanding your model. Regardless, rather than trying to re-explain your model can you please confirm that you've written test code to confirm your mental model? If so, can you please provide this test code? I've provided several test patches proving out my mental model. > As the interrupt is in disabled state it won't be sent to CPU. > Its only when the driver chooses to switch back to interrupt mode we > want to clear the error interrupt latched to start as clean. same is the > case when used as output direction. > > Hope above is clear. Unfortunately, it's still not. :( Can I convince you to provide a test patch and a set of steps that will demonstrate the problem you're worried about? Specifically: a) Maybe you're talking about the initial switch from a plain GPIO input to making it an interrupt for the first time? Are you worried about a phantom interrupt in this case? After patch #1 I think we're safe because pdc_gic_set_type() will always clear the interrupt, right? b) You say "switch back to interrupt mode". Are you imagining that a driver does something like this: request_irq(); ... free_irq(); ... request_irq(); If you're worried about that then we can implement irq_shutdown() for PDC and then make sure we clear on the first enable after a shutdown, I guess? c) Maybe when you say "switch back to interrupt mode" you mean something else? If you are talking about muxing away and then muxing back then I think we already have this covered. If you are talking about masking/unmasking then the whole point is that we _do_ want interrupts latched while masked, right? OK, I'm going to send out a v3 just to get the already-identified problems fixed and also to allow landing of patch #1 in the series, which I think is all agreed upon. My request to you is that if you think my code misses a specific case to provide some test patches to demonstrate that case. -Doug