LKML Archive on lore.kernel.org
 help / Atom feed
From: Lina Iyer <ilina@codeaurora.org>
To: Marc Zyngier <marc.zyngier@arm.com>
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
Date: Thu, 2 Aug 2018 06:58:27 -0600
Message-ID: <20180802125827.GB27850@codeaurora.org> (raw)
In-Reply-To: <86sh3xw7m9.wl-marc.zyngier@arm.com>

On Thu, Aug 02 2018 at 01:27 -0600, Marc Zyngier wrote:
>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.
>
Sure, yes. Sorry for not registering your point in my response.
Once woken up we should see the level interrupt in TLMM.

-- Lina

>>
>> >> > 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.

  reply index

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-01  2:00 [PATCH RESEND RFC 0/4] Wakeup GPIO support for SDM845 SoC Lina Iyer
2018-08-01  2:00 ` [PATCH RESEND RFC 1/4] drivers: pinctrl: qcom: add wakeup capability to GPIO Lina Iyer
2018-08-01  6:31   ` Marc Zyngier
2018-08-01 19:45     ` Lina Iyer
2018-08-01 22:36       ` Bjorn Andersson
2018-08-02  2:10         ` Lina Iyer
2018-08-02  6:08       ` Marc Zyngier
2018-08-02  6:51         ` Lina Iyer
2018-08-02  7:27           ` Marc Zyngier
2018-08-02 12:58             ` Lina Iyer [this message]
2018-08-08  6:05               ` Stephen Boyd
2018-08-08  6:26                 ` Marc Zyngier
2018-08-09 17:30                   ` Stephen Boyd
2018-08-10  7:45                     ` Marc Zyngier
2018-08-10 15:06                       ` Stephen Boyd
2018-08-10 16:15                         ` Lina Iyer
2018-08-01  2:00 ` [PATCH RESEND RFC 2/4] drivers: pinctrl: qcom: add wakeup gpio map for sdm845 Lina Iyer
2018-08-01  8:42   ` Marc Zyngier
2018-08-01 20:04     ` Lina Iyer
2018-08-13 19:41       ` Lina Iyer
2018-08-14  9:26         ` Marc Zyngier
2018-08-01  2:00 ` [PATCH RESEND RFC 3/4] arm64: dts: msm: add PDC device bindings " Lina Iyer
2018-08-01  2:00 ` [PATCH RESEND RFC 4/4] arm64: dts: qcom: add wake up interrupts for GPIOs Lina Iyer

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180802125827.GB27850@codeaurora.org \
    --to=ilina@codeaurora.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=evgreen@chromium.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marc.zyngier@arm.com \
    --cc=rnayak@codeaurora.org \
    --cc=rplsssn@codeaurora.org \
    --cc=swboyd@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox