Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 02 Aug 2018 08:27:42 +0100
Message-ID: <86sh3xw7m9.wl-marc.zyngier@arm.com>
From:   Marc Zyngier <marc.zyngier@arm.com>
To:     Lina Iyer <ilina@codeaurora.org>
Cc:     swboyd@chromium.org, evgreen@chromium.org,
        linus.walleij@linaro.org, bjorn.andersson@linaro.org,
        rplsssn@codeaurora.org, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, rnayak@codeaurora.org,
        devicetree@vger.kernel.org
Subject: Re: [PATCH RESEND RFC 1/4] drivers: pinctrl: qcom: add wakeup capability to GPIO
In-Reply-To: <20180802065104.GA27850@codeaurora.org>
References: <20180801020021.9782-1-ilina@codeaurora.org>
        <20180801020021.9782-2-ilina@codeaurora.org>
        <86600uy4vh.wl-marc.zyngier@arm.com>
        <20180801194538.GA6422@codeaurora.org>
        <86wot9wb9u.wl-marc.zyngier@arm.com>
        <20180802065104.GA27850@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

On Thu, 02 Aug 2018 07:51:04 +0100,
Lina Iyer <ilina@codeaurora.org> wrote:
> 
> On Thu, Aug 02 2018 at 00:08 -0600, Marc Zyngier wrote:
> > Hi Lina,
> > 
> > On Wed, 01 Aug 2018 20:45:38 +0100,
> > Lina Iyer <ilina@codeaurora.org> wrote:
> >> 
> >> Thanks for the feedback, Marc.
> >> 
> >> On Wed, Aug 01 2018 at 00:31 -0600, Marc Zyngier wrote:
> >> > On Wed, 01 Aug 2018 03:00:18 +0100,
> >> > Lina Iyer <ilina@codeaurora.org> wrote:
> >> >>
> >> >> +static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
> >> >> +{
> >> >> +	struct irq_data *irqd = data;
> >> >> +	struct irq_desc *desc = irq_data_to_desc(irqd);
> >> >> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> >> >> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> >> >> +	int irq_pin = irq_find_mapping(gc->irq.domain, irqd->hwirq);
> >> >> +
> >> >> +	chained_irq_enter(chip, desc);
> >> >> +	generic_handle_irq(irq_pin);
> >> >> +	chained_irq_exit(chip, desc);
> >> >
> >> > That's crazy. I'm not even commenting on the irq handler vs chained
> >> > irqchip thing, but directly calling into a completely different part
> >> > of the irq hierarchy makes me feel nauseous,
> >> >
> >> I know the sentiment; I am not too happy about it myself. Explanation
> >> below.
> >> 
> >> > Why isn't the interrupt still pending at the pinctrl level? Looking at
> >> > the diagram in the cover letter, I'd have hoped that the signal routed
> >> > to the PDC would wakeup the GIC, but that by virtue of being *also*
> >> > wired to the TLMM, the interrupt would be handled via the normal path.
> >> >
> >> The short answer: TLMM is not active to sense a wakeup interrupt.
> > 
> > I get that bit. See below for the bit I don't get.
> > 
> >> When we enter system sleep mode, the TLMM and the GIC are powered off
> >> and the PDC is the only powered-on interrupt controller. The IRQs
> >> configured at the PDC are the only ones capable of waking the system.
> >> Upon sensing the interrupt, the PDC intiates a system wakeup and replays
> >> the interrupt to the GIC. The GIC relays that to AP. Unfortunately, the
> >> interrupts from the GPIO do not trigger the TLMM summary line. Therefore
> >> this handler needs to figure out what GPIO caused the wakeup and notify
> >> the corresponding driver.
> > 
> > That's most bizarre. What causes the TLMM output line not to reflect
> > the fact that an input is asserted?
> There is a parallel line that is directed from the PIN directly to the
> PDC in addition to the TLMM. This line does not go through the TLMM.

I got that from the cover letter.

> 
> Active path						_____
>                 /-------------- > [ TLMM ] --------> |     |
>            [ Device GPIO ]	   	     summary   | GIC | ==>
> 		\				       |     |
> 		  ----------------> [ PDC ] ---------> |_____|
> Wakeup path	     GPIO as IRQ		IRQ
> 
> > I understand that it has been
> > turned off, but surely the PDC wakes it up at the same time as the
> > GIC, and it should realise that there is something pending...
> > 
> PDC is always-on and when it detects any interrupt, will wakeup the GIC
> and then replay the interrupt line to the GIC. This interrupt line is
> not the summary line, but a separate line from GPIO/PIN to the PDC.
> 
> Since the TLMM was also in low power mode, when the GIC was powered
> down, the TLMM never sees the IRQ and therefore will not send the
> summary line to GIC. The wakeup path is GPIO -> PDC -> GIC.

Sure. But once woken up (GIC *and* TLMM), the gpio line (which I
assume is level) is still high at the TLMM input. So why isn't it
registering that state once it has been woken up?

I can understand that it would be missing an edge. But that doesn't
hold for level signalling.

> 
> >> > Why isn't that the case? And if that's because the HW is broken and
> >> > doesn't buffer edge interrupts, why can't you use the resend mechanism
> >> > instead?
> >> >
> >> The PDC hardware can replay the interrupts accurately. However, it will
> >> replay only the pin and its not the TLMM summary IRQ. The handler here,
> >> needs to notify the driver that the wakeup interrupt happened and needs
> >> to take action. If I could trip the summary IRQ in this handler that
> >> would work too. Can it be done?
> > 
> > So the TLMM has indeed noticed the interrupt and you can read the TLMM
> > status registers to find out what fired?
> I think that's where it is probably confusing. The TLMM will not see the
> interrupt because it was in low power mode.

See above. I can buy that for edge, but not level.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.