*[PATCH RESEND RFC 0/4] Wakeup GPIO support for SDM845 SoC@ 2018-08-01 2:00 Lina Iyer
2018-08-01 2:00 ` [PATCH RESEND RFC 1/4] drivers: pinctrl: qcom: add wakeup capability to GPIO Lina Iyer
` (3 more replies)0 siblings, 4 replies; 23+ messages in thread
From: Lina Iyer @ 2018-08-01 2:00 UTC (permalink / raw)
To: marc.zyngier, swboyd, evgreen, linus.walleij, bjorn.andersson
Cc: rplsssn, linux-kernel, linux-arm-msm, rnayak, devicetree, Lina Iyer
Hi,
This is an attempt at a solution to enable wake up from suspend and deep idle
using GPIO as a wakeup source. The 845 uses a new interrupt controller (PDC)
that lies in the always-on domain and can sense interrupts that are routed to
it, when the GIC is powered off. It would then wakeup the GIC and replay the
interrupt which would then be relayed to the AP. The PDC interrupt controller
driver is merged upstream [1],[2]. The following set of patches extends the
wakeup capability to GPIOs using the PDC. The TLMM pinctrl driver for the SoC
available at [3].
The complexity with the solution stems from the fact that only a selected few
GPIO lines are routed to the PDC in addition the TLMMs. They are also from
different banks on the pinctrl and the TLMM summary line is not routed to the
PDC. Hence the PDC cannot be considered as parent of the TLMM irqchip (or can
we ?). This is what it looks like -
[ PIN ] -----[ TLMM ]---------------> [ GIC ] ---> [ CPU ]
| <summary IRQ> ^
| |
----------------------------------> [ PDC ]
<wakeup IRQs>
I had a brief discussion with Linus on this and the idea implemented is based
on his suggestion.
When an IRQ (let's call this latent IRQ) for a GPIO is requested, the
->irq_request_resources() is used by the TLMM driver to request a PDC pin. The
PDC pin associated with the GPIO is read from a static map available in the
pinctrl-sdm845.c. (I think there should be a better location than a static map,
more on that later). Knowing the PDC pin from the map, we could look up the DT
bindings and request the PDC interrupt with the same trigger mask as the
interrupt requested. The ->set_type and ->set_wake are also trapped to set the
PDC IRQ's polarity and enable it when the latent IRQ is requested. When the PDC
detects the interrupt at suspend, it wakes up the GIC and replays the wakeup
IRQ. The GPIO handler function for the latent IRQ is invoked in turn.
Please review these patches and your inputs would be greatly appreciated and
(kindly) let me know if I have committed any blunders with this approach. There
is definitely opportunity to improve the location of the static GPIO-PDC pin
map. We could possibly put it as an data argument in the interrupts definition
of the PDC or with interrupt names. Also, I am still sorting out some issues
with the IRQ handling part of these patches. And I am unsure of how to set the
polarity of the PDC pin without locking, since we are not in hierarchy with the
PDC interrupt controller. Again, your inputs on these would be greatly helpful.
Thanks,
Lina
[1]. drivers/irqchip/qcom-pdc.c
[2]. Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
[3]. drivers/pinctrl/qcom/pinctrl-msm.c
Lina Iyer (4):
drivers: pinctrl: qcom: add wakeup capability to GPIO
drivers: pinctrl: qcom: add wakeup gpio map for sdm845
arm64: dts: msm: add PDC device bindings for sdm845
arm64: dts: qcom: add wake up interrupts for GPIOs
arch/arm64/boot/dts/qcom/sdm845.dtsi | 78 ++++++++++++
drivers/pinctrl/qcom/pinctrl-msm.c | 163 ++++++++++++++++++++++++++
drivers/pinctrl/qcom/pinctrl-msm.h | 14 +++
drivers/pinctrl/qcom/pinctrl-sdm845.c | 76 ++++++++++++
4 files changed, 331 insertions(+)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^permalinkrawreply [flat|nested] 23+ messages in thread

*Re: [PATCH RESEND RFC 1/4] drivers: pinctrl: qcom: add wakeup capability to GPIO
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 Zyngier1 sibling, 1 reply; 23+ messages in thread
From: Bjorn Andersson @ 2018-08-01 22:36 UTC (permalink / raw)
To: Lina Iyer
Cc: Marc Zyngier, swboyd, evgreen, linus.walleij, rplsssn,
linux-kernel, linux-arm-msm, rnayak, devicetree
On Wed 01 Aug 12:45 PDT 2018, Lina Iyer 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:
[..]
> > 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?
>
Does this means that the intr_status_reg will not hold the information
about the interrupt events that occurred before we powered on the TLMM
again? Or is the problem limited to it not triggering the TLMM IRQ?
Can the PDC (and MPM) be used in the non-sleep use cases as well?
Regards,
Bjorn
^permalinkrawreply [flat|nested] 23+ messages in thread

*Re: [PATCH RESEND RFC 1/4] drivers: pinctrl: qcom: add wakeup capability to GPIO
2018-08-01 22:36 ` Bjorn Andersson@ 2018-08-02 2:10 ` Lina Iyer0 siblings, 0 replies; 23+ messages in thread
From: Lina Iyer @ 2018-08-02 2:10 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Marc Zyngier, swboyd, evgreen, linus.walleij, rplsssn,
linux-kernel, linux-arm-msm, rnayak, devicetree
On Wed, Aug 01 2018 at 16:38 -0600, Bjorn Andersson wrote:
>On Wed 01 Aug 12:45 PDT 2018, Lina Iyer 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:
>[..]
>> > 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?
>>
>
>Does this means that the intr_status_reg will not hold the information
>about the interrupt events that occurred before we powered on the TLMM
>again? Or is the problem limited to it not triggering the TLMM IRQ?
>
It doesn't. The TLMM is in low power state and cannot sense interrupts.
>Can the PDC (and MPM) be used in the non-sleep use cases as well?
>
Yes. We use PDC to modify polarity of falling edge and low level
interrupts to rising edge and high level interrupt so they may be sensed
at the GIC. I am not sure about the MPM. I suspect it is not.
-- Lina
^permalinkrawreply [flat|nested] 23+ messages in thread

*Re: [PATCH RESEND RFC 1/4] drivers: pinctrl: qcom: add wakeup capability to GPIO
2018-08-01 19:45 ` Lina Iyer
2018-08-01 22:36 ` Bjorn Andersson@ 2018-08-02 6:08 ` Marc Zyngier
2018-08-02 6:51 ` Lina Iyer1 sibling, 1 reply; 23+ messages in thread
From: Marc Zyngier @ 2018-08-02 6:08 UTC (permalink / raw)
To: Lina Iyer
Cc: swboyd, evgreen, linus.walleij, bjorn.andersson, rplsssn,
linux-kernel, linux-arm-msm, rnayak, devicetree
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? 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...
>
> > 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? The question remains: why
isn't it generating any output if it knows something as fired? The
output line is level, right? What you describe would make sense if it
was generating an edge, but that'd really be a terrible design...
As for tripping the summary interrupt, see check_irq_resend(). This
will only work if this summary interrupt is edge.
Thanks,
M.
--
Jazz is not dead, it just smell funny.
^permalinkrawreply [flat|nested] 23+ messages in thread

*Re: [PATCH RESEND RFC 1/4] drivers: pinctrl: qcom: add wakeup capability to GPIO
2018-08-02 6:08 ` Marc Zyngier@ 2018-08-02 6:51 ` Lina Iyer
2018-08-02 7:27 ` Marc Zyngier0 siblings, 1 reply; 23+ messages in thread
From: Lina Iyer @ 2018-08-02 6:51 UTC (permalink / raw)
To: Marc Zyngier
Cc: swboyd, evgreen, linus.walleij, bjorn.andersson, rplsssn,
linux-kernel, linux-arm-msm, rnayak, devicetree
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.
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.
>> > 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.
>The question remains: why
>isn't it generating any output if it knows something as fired? The
>output line is level, right? What you describe would make sense if it
>was generating an edge, but that'd really be a terrible design...
>
>As for tripping the summary interrupt, see check_irq_resend(). This
>will only work if this summary interrupt is edge.
>
Will explore this.
Thanks Marc.
-- Lina
^permalinkrawreply [flat|nested] 23+ messages in thread

*Re: [PATCH RESEND RFC 1/4] drivers: pinctrl: qcom: add wakeup capability to GPIO
2018-08-02 6:51 ` Lina Iyer@ 2018-08-02 7:27 ` Marc Zyngier
2018-08-02 12:58 ` Lina Iyer0 siblings, 1 reply; 23+ messages in thread
From: Marc Zyngier @ 2018-08-02 7:27 UTC (permalink / raw)
To: Lina Iyer
Cc: swboyd, evgreen, linus.walleij, bjorn.andersson, rplsssn,
linux-kernel, linux-arm-msm, rnayak, devicetree
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.
^permalinkrawreply [flat|nested] 23+ messages in thread

*Re: [PATCH RESEND RFC 1/4] drivers: pinctrl: qcom: add wakeup capability to GPIO
2018-08-02 7:27 ` Marc Zyngier@ 2018-08-02 12:58 ` Lina Iyer
2018-08-08 6:05 ` Stephen Boyd0 siblings, 1 reply; 23+ messages in thread
From: Lina Iyer @ 2018-08-02 12:58 UTC (permalink / raw)
To: Marc Zyngier
Cc: swboyd, evgreen, linus.walleij, bjorn.andersson, rplsssn,
linux-kernel, linux-arm-msm, rnayak, devicetree
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.
^permalinkrawreply [flat|nested] 23+ messages in thread

*Re: [PATCH RESEND RFC 1/4] drivers: pinctrl: qcom: add wakeup capability to GPIO
2018-08-02 12:58 ` Lina Iyer@ 2018-08-08 6:05 ` Stephen Boyd
2018-08-08 6:26 ` Marc Zyngier0 siblings, 1 reply; 23+ messages in thread
From: Stephen Boyd @ 2018-08-08 6:05 UTC (permalink / raw)
To: Lina Iyer, Marc Zyngier
Cc: evgreen, linus.walleij, bjorn.andersson, rplsssn, linux-kernel,
linux-arm-msm, rnayak, devicetree
Quoting Lina Iyer (2018-08-02 05:58:27)
> On Thu, Aug 02 2018 at 01:27 -0600, Marc Zyngier wrote:
> >
> >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.
And the level type gpio interrupt will trigger the TLMM summary
interrupt line after the wakeup? So then the only thing that needs to be
replayed is edge interrupts? How are edge interrupts going to be
replayed?
^permalinkrawreply [flat|nested] 23+ messages in thread

*Re: [PATCH RESEND RFC 1/4] drivers: pinctrl: qcom: add wakeup capability to GPIO
2018-08-08 6:05 ` Stephen Boyd@ 2018-08-08 6:26 ` Marc Zyngier
2018-08-09 17:30 ` Stephen Boyd0 siblings, 1 reply; 23+ messages in thread
From: Marc Zyngier @ 2018-08-08 6:26 UTC (permalink / raw)
To: Stephen Boyd
Cc: Lina Iyer, evgreen, linus.walleij, bjorn.andersson, rplsssn,
linux-kernel, linux-arm-msm, rnayak, devicetree
On Tue, 07 Aug 2018 23:05:07 -0700
Stephen Boyd <swboyd@chromium.org> wrote:
> Quoting Lina Iyer (2018-08-02 05:58:27)
> > On Thu, Aug 02 2018 at 01:27 -0600, Marc Zyngier wrote:
> > >
> > >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.
>
> And the level type gpio interrupt will trigger the TLMM summary
> interrupt line after the wakeup? So then the only thing that needs to be
> replayed is edge interrupts? How are edge interrupts going to be
> replayed?
Level interrupts should be taken care of without doing anything, by the
very nature of being a level signal.
Edge interrupts should be replayed using check_irq_resend() after
taking the right locks and making the interrupt pending. Or, if there
is a way for SW to make the interrupt pending at the TLMM level, to use
that as a way to reinject the interrupt (which would be the preferred
way, as it avoids all kind of ugly locking considerations).
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^permalinkrawreply [flat|nested] 23+ messages in thread

*Re: [PATCH RESEND RFC 1/4] drivers: pinctrl: qcom: add wakeup capability to GPIO
2018-08-08 6:26 ` Marc Zyngier@ 2018-08-09 17:30 ` Stephen Boyd
2018-08-10 7:45 ` Marc Zyngier0 siblings, 1 reply; 23+ messages in thread
From: Stephen Boyd @ 2018-08-09 17:30 UTC (permalink / raw)
To: Marc Zyngier
Cc: Lina Iyer, evgreen, linus.walleij, bjorn.andersson, rplsssn,
linux-kernel, linux-arm-msm, rnayak, devicetree
Quoting Marc Zyngier (2018-08-07 23:26:32)
> On Tue, 07 Aug 2018 23:05:07 -0700
> Stephen Boyd <swboyd@chromium.org> wrote:
>
> > Quoting Lina Iyer (2018-08-02 05:58:27)
> > > On Thu, Aug 02 2018 at 01:27 -0600, Marc Zyngier wrote:
> > > >
> > > >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.
> >
> > And the level type gpio interrupt will trigger the TLMM summary
> > interrupt line after the wakeup? So then the only thing that needs to be
> > replayed is edge interrupts? How are edge interrupts going to be
> > replayed?
>
> Level interrupts should be taken care of without doing anything, by the
> very nature of being a level signal.
Right. I suspect we'll still need to configure the PDC to actually wake
up on the level triggered signal though so PDC needs to be told to
unmask the line.
>
> Edge interrupts should be replayed using check_irq_resend() after
> taking the right locks and making the interrupt pending. Or, if there
> is a way for SW to make the interrupt pending at the TLMM level, to use
> that as a way to reinject the interrupt (which would be the preferred
> way, as it avoids all kind of ugly locking considerations).
>
Ok. Looking at the hardware it seems that I can write the interrupt
status bit directly for an edge type interrupt and that causes the
interrupt handler to run. So that's good news, we can use that ability
to directly inject interrupts here.
^permalinkrawreply [flat|nested] 23+ messages in thread

*Re: [PATCH RESEND RFC 1/4] drivers: pinctrl: qcom: add wakeup capability to GPIO
2018-08-09 17:30 ` Stephen Boyd@ 2018-08-10 7:45 ` Marc Zyngier
2018-08-10 15:06 ` Stephen Boyd0 siblings, 1 reply; 23+ messages in thread
From: Marc Zyngier @ 2018-08-10 7:45 UTC (permalink / raw)
To: Stephen Boyd
Cc: Lina Iyer, evgreen, linus.walleij, bjorn.andersson, rplsssn,
linux-kernel, linux-arm-msm, rnayak, devicetree
On Thu, 09 Aug 2018 18:30:53 +0100,
Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Marc Zyngier (2018-08-07 23:26:32)
> > On Tue, 07 Aug 2018 23:05:07 -0700
> > Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > > Quoting Lina Iyer (2018-08-02 05:58:27)
> > > > On Thu, Aug 02 2018 at 01:27 -0600, Marc Zyngier wrote:
> > > > >
> > > > >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.
> > >
> > > And the level type gpio interrupt will trigger the TLMM summary
> > > interrupt line after the wakeup? So then the only thing that needs to be
> > > replayed is edge interrupts? How are edge interrupts going to be
> > > replayed?
> >
> > Level interrupts should be taken care of without doing anything, by the
> > very nature of being a level signal.
>
> Right. I suspect we'll still need to configure the PDC to actually wake
> up on the level triggered signal though so PDC needs to be told to
> unmask the line.
Surely this can be done at suspend time with the PDC driver tracking
the interrupts that are configured as a wake-up source (although it
needs to track an interrupt that is logically connected to the TLMM,
which sucks).
> > Edge interrupts should be replayed using check_irq_resend() after
> > taking the right locks and making the interrupt pending. Or, if there
> > is a way for SW to make the interrupt pending at the TLMM level, to use
> > that as a way to reinject the interrupt (which would be the preferred
> > way, as it avoids all kind of ugly locking considerations).
> >
>
> Ok. Looking at the hardware it seems that I can write the interrupt
> status bit directly for an edge type interrupt and that causes the
> interrupt handler to run. So that's good news, we can use that ability
> to directly inject interrupts here.
That's pretty good news. It means that if TLMM implements the
irq_set_irqchip_state() API, there isn't muc that needs doing, and
most of the original ugliness can go. At least I hope.
M.
--
Jazz is not dead, it just smell funny.
^permalinkrawreply [flat|nested] 23+ messages in thread

*Re: [PATCH RESEND RFC 1/4] drivers: pinctrl: qcom: add wakeup capability to GPIO
2018-08-10 15:06 ` Stephen Boyd@ 2018-08-10 16:15 ` Lina Iyer0 siblings, 0 replies; 23+ messages in thread
From: Lina Iyer @ 2018-08-10 16:15 UTC (permalink / raw)
To: Stephen Boyd
Cc: Marc Zyngier, evgreen, linus.walleij, bjorn.andersson, rplsssn,
linux-kernel, linux-arm-msm, rnayak, devicetree
On Fri, Aug 10 2018 at 09:06 -0600, Stephen Boyd wrote:
>Quoting Marc Zyngier (2018-08-10 00:45:12)
>> On Thu, 09 Aug 2018 18:30:53 +0100,
>> Stephen Boyd <swboyd@chromium.org> wrote:
>> >
>> > Quoting Marc Zyngier (2018-08-07 23:26:32)
>> > >
>> > > Level interrupts should be taken care of without doing anything, by the
>> > > very nature of being a level signal.
>> >
>> > Right. I suspect we'll still need to configure the PDC to actually wake
>> > up on the level triggered signal though so PDC needs to be told to
>> > unmask the line.
>>
>> Surely this can be done at suspend time with the PDC driver tracking
>> the interrupts that are configured as a wake-up source (although it
>> needs to track an interrupt that is logically connected to the TLMM,
>> which sucks).
>
>The PDC also needs to be configured for wakeups from deep CPU idle
>states where the GIC and TLMM are powered down. Lina, can you confirm
>this?
>
Yes, it will need to be handled as part of CPU idle as well, when the
last CPU powers down.
>Hooking system suspend in that case won't work. Is your hope that we can
>avoid using hierarchical irqdomains here entirely?
>
Well, I wasn't trying to avoid hierarchical irqdomains, there were
restrictions in using it. Not all GPIO pins have parent in PDC and the
ones that have are all not from the same bank either.
-- Lina
^permalinkrawreply [flat|nested] 23+ messages in thread