[PATCH RFC 0/1] QCOM: GPIO IRQ wakeup using PDC irqchip

[PATCH RFC 0/1] QCOM: GPIO IRQ wakeup using PDC irqchip

[Lina Iyer]

Hi all,

This is a second attempt at enabling wakeup using GPIO IRQs. On the QCOM
SDM845i SoC, even when the TLMM gpiochip is powered off the the PDC always-on
interrupt controller can sense the GPIO interrupt as a signal, wakeup the
interrupt and replay the interrupt at the GIC.

Since, the wakeup interrupt is replayed at the GIC and not at the TLMM, it
would have to be considered as a separate interrupt line and not in hierarchy
with the GPIO interrupt line which is chained to the GIC, using a single
summary line.

The earlier approach [1] was based on configuring a PDC interrupt whenever a
GPIO was requested as an IRQ and the PDC irq line was kept disabled. During
suspend and resume, the TLMM line was disabled and the PDC line was enabled.
While this works for suspend/resume, it becomes an additional complexity to
handle when entering a system low power mode where the TLMM could be powered
off when the CPUs are idle.

The approach here is to use PDC interrupt at all times (while keeping the GPIO
irq disabled). The PDC interrupt handler invokes the action handler of the GPIO
IRQ instead. This allows us to avoid interrupt hand-offs while entering idle
states and keeps things very simple. The wake_irq_gpio_handler() is the crux of
this new revision.

This submission does not include the DT bindings and associated documentation.
Once the core design is resolved I will send a complete set with dependencies
for this SoC. Please let me know if this approach would work or do you see any
issues that I may have missed.

Thanks,
Lina

[1]. https://lkml.org/lkml/2018/9/4/846

Lina Iyer (1):
  drivers: pinctrl: qcom: add wakeup capability to GPIO

 drivers/pinctrl/qcom/pinctrl-msm.c | 91 +++++++++++++++++++++++++++++-
 1 file changed, 90 insertions(+), 1 deletion(-)

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Lina Iyer]

QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on
domain can wakeup the SoC, when interrupts and GPIOs are routed to its
interrupt controller. Only select GPIOs that are deemed wakeup capable
are routed to specific PDC pins. During low power state, the pinmux
interrupt controller may be non-functional but the PDC would be. The PDC
can detect the wakeup GPIO is triggered and bring the TLMM to an
operational state.

Interrupts that are level triggered will be detected at the TLMM when
the controller becomes operational. Edge interrupts however need to be
replayed again.

Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ,
but keep it disabled. During suspend, we can enable the PDC IRQ instead
of the GPIO IRQ, which may or not be detected.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v4:
	- Redesign to use PDC interrupts instead of TLMM interrupt
Changes in v3:
	- free action->name
Changes in v2:
	- Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ
Changes in v1:
	- Trigger GPIO in h/w from PDC IRQ handler
	- Avoid big tables for GPIO-PDC map, pick from DT instead
	- Use handler_data
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 91 +++++++++++++++++++++++++++++-
 1 file changed, 90 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 5d72ffad32c2..70b9178eba30 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -719,6 +719,12 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	const struct msm_pingroup *g;
 	unsigned long flags;
 	u32 val;
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
+
+	if (pdc_irqd) {
+		irq_set_irq_type(pdc_irqd->irq, type);
+		goto handler;
+	}
 
 	g = &pctrl->soc->groups[d->hwirq];
 
@@ -798,6 +804,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 
+handler:
 	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
 		irq_set_handler_locked(d, handle_level_irq);
 	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
@@ -811,9 +818,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
 	unsigned long flags;
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
+	if (pdc_irqd)
+		irq_set_irq_wake(pdc_irqd->irq, on);
+
 	irq_set_irq_wake(pctrl->irq, on);
 
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
@@ -895,6 +906,83 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
 	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
 }
 
+static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
+{
+	struct irq_data *irqd = data;
+	struct irq_desc *desc = irq_to_desc(irqd->irq);
+
+	desc->handle_irq(desc);
+
+	return IRQ_HANDLED;
+}
+
+static int msm_gpio_pdc_pin_request(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	struct platform_device *pdev = to_platform_device(pctrl->dev);
+	const char *pin_name;
+	int irq;
+	int ret;
+
+	pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq);
+	if (!pin_name)
+		return -ENOMEM;
+
+	irq = platform_get_irq_byname(pdev, pin_name);
+	if (irq < 0) {
+		kfree(pin_name);
+		return 0;
+	}
+
+	ret = request_irq(irq, wake_irq_gpio_handler, irqd_get_trigger_type(d),
+			  pin_name, d);
+	if (ret) {
+		pr_warn("Error requesting PDC interrupt %d for %s\n",
+			irq, pin_name);
+		kfree(pin_name);
+		return ret;
+	}
+	irq_set_handler_data(d->irq, irq_get_irq_data(irq));
+
+	return 0;
+}
+
+static int msm_gpio_pdc_pin_release(struct irq_data *d)
+{
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
+	const void *name;
+
+	if (pdc_irqd) {
+		irq_set_handler_data(d->irq, NULL);
+		name = free_irq(pdc_irqd->irq, d);
+		kfree(name);
+	}
+
+	return 0;
+}
+
+static int msm_gpio_irq_reqres(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
+		dev_err(gc->parent, "unable to lock HW IRQ %lu for IRQ\n",
+			irqd_to_hwirq(d));
+		return -EINVAL;
+	}
+
+	return msm_gpio_pdc_pin_request(d);
+}
+
+static void msm_gpio_irq_relres(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	msm_gpio_pdc_pin_release(d);
+	gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
+}
+
 static int msm_gpio_init(struct msm_pinctrl *pctrl)
 {
 	struct gpio_chip *chip;
@@ -919,6 +1007,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
 	pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
 	pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
+	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
+	pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
@@ -1072,4 +1162,3 @@ int msm_pinctrl_remove(struct platform_device *pdev)
 	return 0;
 }
 EXPORT_SYMBOL(msm_pinctrl_remove);
-
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Lina Iyer]

Hi folks,

On Wed, Oct 10 2018 at 18:30 -0600, Lina Iyer wrote:
>QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on
>domain can wakeup the SoC, when interrupts and GPIOs are routed to its
>interrupt controller. Only select GPIOs that are deemed wakeup capable
>are routed to specific PDC pins. During low power state, the pinmux
>interrupt controller may be non-functional but the PDC would be. The PDC
>can detect the wakeup GPIO is triggered and bring the TLMM to an
>operational state.
>
>Interrupts that are level triggered will be detected at the TLMM when
>the controller becomes operational. Edge interrupts however need to be
>replayed again.
>
>Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ,
>but keep it disabled. During suspend, we can enable the PDC IRQ instead
>of the GPIO IRQ, which may or not be detected.
>
>Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>---
>Changes in v4:
>	- Redesign to use PDC interrupts instead of TLMM interrupt
>Changes in v3:
>	- free action->name
>Changes in v2:
>	- Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ
>Changes in v1:
>	- Trigger GPIO in h/w from PDC IRQ handler
>	- Avoid big tables for GPIO-PDC map, pick from DT instead
>	- Use handler_data
>---
> drivers/pinctrl/qcom/pinctrl-msm.c | 91 +++++++++++++++++++++++++++++-
> 1 file changed, 90 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>index 5d72ffad32c2..70b9178eba30 100644
>--- a/drivers/pinctrl/qcom/pinctrl-msm.c
>+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>@@ -719,6 +719,12 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> 	const struct msm_pingroup *g;
> 	unsigned long flags;
> 	u32 val;
>+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>+
>+	if (pdc_irqd) {
>+		irq_set_irq_type(pdc_irqd->irq, type);
>+		goto handler;
>+	}
>
> 	g = &pctrl->soc->groups[d->hwirq];
>
>@@ -798,6 +804,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>
> 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>
>+handler:
> 	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
> 		irq_set_handler_locked(d, handle_level_irq);
> 	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
>@@ -811,9 +818,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
> 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> 	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> 	unsigned long flags;
>+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>
> 	raw_spin_lock_irqsave(&pctrl->lock, flags);
>
>+	if (pdc_irqd)
>+		irq_set_irq_wake(pdc_irqd->irq, on);
>+
> 	irq_set_irq_wake(pctrl->irq, on);
>
> 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>@@ -895,6 +906,83 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
> 	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
> }
>
>+static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
>+{
>+	struct irq_data *irqd = data;
>+	struct irq_desc *desc = irq_to_desc(irqd->irq);
>+
>+	desc->handle_irq(desc);
Do we see any problem calling handle_irq()?

Thanks,
Lina
>+
>+	return IRQ_HANDLED;
>+}
>+
>+static int msm_gpio_pdc_pin_request(struct irq_data *d)
>+{
>+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>+	struct platform_device *pdev = to_platform_device(pctrl->dev);
>+	const char *pin_name;
>+	int irq;
>+	int ret;
>+
>+	pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq);
>+	if (!pin_name)
>+		return -ENOMEM;
>+
>+	irq = platform_get_irq_byname(pdev, pin_name);
>+	if (irq < 0) {
>+		kfree(pin_name);
>+		return 0;
>+	}
>+
>+	ret = request_irq(irq, wake_irq_gpio_handler, irqd_get_trigger_type(d),
>+			  pin_name, d);
>+	if (ret) {
>+		pr_warn("Error requesting PDC interrupt %d for %s\n",
>+			irq, pin_name);
>+		kfree(pin_name);
>+		return ret;
>+	}
>+	irq_set_handler_data(d->irq, irq_get_irq_data(irq));
>+
>+	return 0;
>+}
>+
>+static int msm_gpio_pdc_pin_release(struct irq_data *d)
>+{
>+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>+	const void *name;
>+
>+	if (pdc_irqd) {
>+		irq_set_handler_data(d->irq, NULL);
>+		name = free_irq(pdc_irqd->irq, d);
>+		kfree(name);
>+	}
>+
>+	return 0;
>+}
>+
>+static int msm_gpio_irq_reqres(struct irq_data *d)
>+{
>+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>+
>+	if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
>+		dev_err(gc->parent, "unable to lock HW IRQ %lu for IRQ\n",
>+			irqd_to_hwirq(d));
>+		return -EINVAL;
>+	}
>+
>+	return msm_gpio_pdc_pin_request(d);
>+}
>+
>+static void msm_gpio_irq_relres(struct irq_data *d)
>+{
>+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>+
>+	msm_gpio_pdc_pin_release(d);
>+	gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
>+}
>+
> static int msm_gpio_init(struct msm_pinctrl *pctrl)
> {
> 	struct gpio_chip *chip;
>@@ -919,6 +1007,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
> 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
> 	pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
> 	pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
>+	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
>+	pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
>
> 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
> 	if (ret) {
>@@ -1072,4 +1162,3 @@ int msm_pinctrl_remove(struct platform_device *pdev)
> 	return 0;
> }
> EXPORT_SYMBOL(msm_pinctrl_remove);
>-
>--
>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>a Linux Foundation Collaborative Project
>

Re: [PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Marc Zyngier]

Hi Lina,

On 19/10/18 16:32, Lina Iyer wrote:
> Hi folks,
> 
> On Wed, Oct 10 2018 at 18:30 -0600, Lina Iyer wrote:
>> QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on
>> domain can wakeup the SoC, when interrupts and GPIOs are routed to its
>> interrupt controller. Only select GPIOs that are deemed wakeup capable
>> are routed to specific PDC pins. During low power state, the pinmux
>> interrupt controller may be non-functional but the PDC would be. The PDC
>> can detect the wakeup GPIO is triggered and bring the TLMM to an
>> operational state.
>>
>> Interrupts that are level triggered will be detected at the TLMM when
>> the controller becomes operational. Edge interrupts however need to be
>> replayed again.
>>
>> Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ,
>> but keep it disabled. During suspend, we can enable the PDC IRQ instead
>> of the GPIO IRQ, which may or not be detected.
>>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>> Changes in v4:
>> 	- Redesign to use PDC interrupts instead of TLMM interrupt
>> Changes in v3:
>> 	- free action->name
>> Changes in v2:
>> 	- Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ
>> Changes in v1:
>> 	- Trigger GPIO in h/w from PDC IRQ handler
>> 	- Avoid big tables for GPIO-PDC map, pick from DT instead
>> 	- Use handler_data
>> ---
>> drivers/pinctrl/qcom/pinctrl-msm.c | 91 +++++++++++++++++++++++++++++-
>> 1 file changed, 90 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index 5d72ffad32c2..70b9178eba30 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>> @@ -719,6 +719,12 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>> 	const struct msm_pingroup *g;
>> 	unsigned long flags;
>> 	u32 val;
>> +	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>> +
>> +	if (pdc_irqd) {
>> +		irq_set_irq_type(pdc_irqd->irq, type);
>> +		goto handler;
>> +	}
>>
>> 	g = &pctrl->soc->groups[d->hwirq];
>>
>> @@ -798,6 +804,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>>
>> 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>>
>> +handler:
>> 	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
>> 		irq_set_handler_locked(d, handle_level_irq);
>> 	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
>> @@ -811,9 +818,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
>> 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> 	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>> 	unsigned long flags;
>> +	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>>
>> 	raw_spin_lock_irqsave(&pctrl->lock, flags);
>>
>> +	if (pdc_irqd)
>> +		irq_set_irq_wake(pdc_irqd->irq, on);
>> +
>> 	irq_set_irq_wake(pctrl->irq, on);
>>
>> 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>> @@ -895,6 +906,83 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
>> 	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
>> }
>>
>> +static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
>> +{
>> +	struct irq_data *irqd = data;
>> +	struct irq_desc *desc = irq_to_desc(irqd->irq);
>> +
>> +	desc->handle_irq(desc);
> Do we see any problem calling handle_irq()?

Good timing, I was just looking at this.

One thing I can see is that you will end-up calling the EOI callback on
the root interrupt controller (the GIC), thus writing to ICC_EOIR1_EL1.

But you've never acked this interrupt the first place by reading
ICC_IAR1_EL1, and that puts you violently out of spec, according to the
GICv3 spec (8.2.10), which reads:

"A write to this register must correspond to the most recent valid read
by this PE from an Interrupt Acknowledge Register, and must correspond
to the INTID that was read from ICC_IAR1_EL1, otherwise the system
behavior is UNPREDICTABLE."

Here, you definitely risk the sanity of the CPU interface state machine.

So stepping back a bit: At some point, you had a version that just
relied on regenerating edge interrupts by writing to some register
(knowing that level interrupts are safe by definition). Why isn't that
the right solution? It'd avoid the above minefield by just letting the
HW do its thing...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Lina Iyer]

On Fri, Oct 19 2018 at 09:53 -0600, Marc Zyngier wrote:
>Hi Lina,
>
>On 19/10/18 16:32, Lina Iyer wrote:
>> Hi folks,
>>
>> On Wed, Oct 10 2018 at 18:30 -0600, Lina Iyer wrote:

[...]

>>> +static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
>>> +{
>>> +	struct irq_data *irqd = data;
>>> +	struct irq_desc *desc = irq_to_desc(irqd->irq);
>>> +
>>> +	desc->handle_irq(desc);
>> Do we see any problem calling handle_irq()?
>
>Good timing, I was just looking at this.
>
:) Thanks for your time.

>One thing I can see is that you will end-up calling the EOI callback on
>the root interrupt controller (the GIC), thus writing to ICC_EOIR1_EL1.
>
>But you've never acked this interrupt the first place by reading
>ICC_IAR1_EL1, and that puts you violently out of spec, according to the
>GICv3 spec (8.2.10), which reads:
>
>"A write to this register must correspond to the most recent valid read
>by this PE from an Interrupt Acknowledge Register, and must correspond
>to the INTID that was read from ICC_IAR1_EL1, otherwise the system
>behavior is UNPREDICTABLE."
>
>Here, you definitely risk the sanity of the CPU interface state machine.
>
Oh, thanks Marc. Will look into it. The problem is because I call
handle_irq() directly for the GPIO IRQ which is not triggered but we end
up mask, eoi etc.

How about calling handle_simple_irq(), which doesn't seem to the IRQ
registers?

>So stepping back a bit: At some point, you had a version that just
>relied on regenerating edge interrupts by writing to some register
>(knowing that level interrupts are safe by definition). Why isn't that
>the right solution? It'd avoid the above minefield by just letting the
>HW do its thing...
>
There are some unnecessary complexity with the approach that we are
trying to avoid.

The TLMM may or not may not be powered off (depending on the SoC state)
and Linux has no control on it. The PDC will remain powered on but we
don't want the PDC interrupt enabled always, since we will receive to
interrupts (one from TLMM and one from PDC) for every event. So we have
to keep the PDC interrupt configured as wakeup interrupt but operate on
the fact that when we go into suspend or cpuidle we will have to switch
to enabling the PDC interrupt and disabling the GPIO IRQ and swap back
when we resume. This dance is harder with cpuidle (notifying the TLMM
driver, when all the CPUs are in idle) than suspend/resume which has
nice callbacks and is what we are trying to avoid but using the PDC
interrupt always.

Thanks,
Lina

Re: [PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Marc Zyngier]

On Fri, 19 Oct 2018 20:47:12 +0100,
Lina Iyer <ilina@codeaurora.org> wrote:
> 
> On Fri, Oct 19 2018 at 09:53 -0600, Marc Zyngier wrote:
> > Hi Lina,
> > 
> > On 19/10/18 16:32, Lina Iyer wrote:
> >> Hi folks,
> >> 
> >> On Wed, Oct 10 2018 at 18:30 -0600, Lina Iyer wrote:
> 
> [...]
> 
> >>> +static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
> >>> +{
> >>> +	struct irq_data *irqd = data;
> >>> +	struct irq_desc *desc = irq_to_desc(irqd->irq);
> >>> +
> >>> +	desc->handle_irq(desc);
> >> Do we see any problem calling handle_irq()?
> > 
> > Good timing, I was just looking at this.
> > 
> :) Thanks for your time.
> 
> > One thing I can see is that you will end-up calling the EOI callback on
> > the root interrupt controller (the GIC), thus writing to ICC_EOIR1_EL1.
> > 
> > But you've never acked this interrupt the first place by reading
> > ICC_IAR1_EL1, and that puts you violently out of spec, according to the
> > GICv3 spec (8.2.10), which reads:
> > 
> > "A write to this register must correspond to the most recent valid read
> > by this PE from an Interrupt Acknowledge Register, and must correspond
> > to the INTID that was read from ICC_IAR1_EL1, otherwise the system
> > behavior is UNPREDICTABLE."
> > 
> > Here, you definitely risk the sanity of the CPU interface state machine.
> > 
> Oh, thanks Marc. Will look into it. The problem is because I call
> handle_irq() directly for the GPIO IRQ which is not triggered but we end
> up mask, eoi etc.
> 
> How about calling handle_simple_irq(), which doesn't seem to the IRQ
> registers?

The problem is that you cannot decide to use another flow handler, as
this handler is a constraint imposed by the root interrupt
controller. You can overload it, but you then need to make sure that
the interrupt will *never* fire at the GIC level, ever. 

Can you actually enforce this?

Assuming you can, this could work. But then the subsequent question
is: Why do you have the interrupt at the TLMM level at all? Overall,
I'm a bit worried of this "now you see me, now you don't" kind of
game behind the kernel back. Is there a way we can stop playing that
game and stick to one single path for interrupt delivery?

> > So stepping back a bit: At some point, you had a version that just
> > relied on regenerating edge interrupts by writing to some register
> > (knowing that level interrupts are safe by definition). Why isn't that
> > the right solution? It'd avoid the above minefield by just letting the
> > HW do its thing...
> > 
> There are some unnecessary complexity with the approach that we are
> trying to avoid.
> 
> The TLMM may or not may not be powered off (depending on the SoC state)
> and Linux has no control on it. The PDC will remain powered on but we
> don't want the PDC interrupt enabled always, since we will receive to
> interrupts (one from TLMM and one from PDC) for every event. So we have
> to keep the PDC interrupt configured as wakeup interrupt but operate on
> the fact that when we go into suspend or cpuidle we will have to switch
> to enabling the PDC interrupt and disabling the GPIO IRQ and swap back
> when we resume. This dance is harder with cpuidle (notifying the TLMM
> driver, when all the CPUs are in idle) than suspend/resume which has
> nice callbacks and is what we are trying to avoid but using the PDC
> interrupt always.

It looks to me that the way this logic is split across two drivers is
a major cause of headache. My advise is that either you have one
single point of interrupt handling for such interrupt, or you force a
TLMM wake-up on such an event, forcing it to handle the interrupt the
"normal way"...

Thanks,

	M.

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

Re: [PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Lina Iyer]

On Mon, Oct 22 2018 at 03:27 -0600, Marc Zyngier wrote:
>On Fri, 19 Oct 2018 20:47:12 +0100,
>Lina Iyer <ilina@codeaurora.org> wrote:
>>
>> On Fri, Oct 19 2018 at 09:53 -0600, Marc Zyngier wrote:
>> > Hi Lina,
>> >
>> > On 19/10/18 16:32, Lina Iyer wrote:
>> >> Hi folks,
>> >>
>> >> On Wed, Oct 10 2018 at 18:30 -0600, Lina Iyer wrote:
>>
>> [...]
>>
>> >>> +static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
>> >>> +{
>> >>> +	struct irq_data *irqd = data;
>> >>> +	struct irq_desc *desc = irq_to_desc(irqd->irq);
>> >>> +
>> >>> +	desc->handle_irq(desc);
>> >> Do we see any problem calling handle_irq()?
>> >
>> > Good timing, I was just looking at this.
>> >
>> :) Thanks for your time.
>>
>> > One thing I can see is that you will end-up calling the EOI callback on
>> > the root interrupt controller (the GIC), thus writing to ICC_EOIR1_EL1.
>> >
>> > But you've never acked this interrupt the first place by reading
>> > ICC_IAR1_EL1, and that puts you violently out of spec, according to the
>> > GICv3 spec (8.2.10), which reads:
>> >
>> > "A write to this register must correspond to the most recent valid read
>> > by this PE from an Interrupt Acknowledge Register, and must correspond
>> > to the INTID that was read from ICC_IAR1_EL1, otherwise the system
>> > behavior is UNPREDICTABLE."
>> >
>> > Here, you definitely risk the sanity of the CPU interface state machine.
>> >
>> Oh, thanks Marc. Will look into it. The problem is because I call
>> handle_irq() directly for the GPIO IRQ which is not triggered but we end
>> up mask, eoi etc.
>>
>> How about calling handle_simple_irq(), which doesn't seem to the IRQ
>> registers?
>
>The problem is that you cannot decide to use another flow handler, as
>this handler is a constraint imposed by the root interrupt
>controller. You can overload it, but you then need to make sure that
>the interrupt will *never* fire at the GIC level, ever.
>
>Can you actually enforce this?
>
Yes. With this appraoch in msm_gpio_irq_set_type(), we skip writing to
the GPIO's IRQ registers and it will not trigger an interrupt.

>Assuming you can, this could work. But then the subsequent question
>is: Why do you have the interrupt at the TLMM level at all? Overall,
>I'm a bit worried of this "now you see me, now you don't" kind of
>game behind the kernel back. Is there a way we can stop playing that
>game and stick to one single path for interrupt delivery?
>
The TLMM summary line will be triggered for other GPIOs that are not
wakeup capable and therefore will not have a parallel PDC interrupt.

>> > So stepping back a bit: At some point, you had a version that just
>> > relied on regenerating edge interrupts by writing to some register
>> > (knowing that level interrupts are safe by definition). Why isn't that
>> > the right solution? It'd avoid the above minefield by just letting the
>> > HW do its thing...
>> >
>> There are some unnecessary complexity with the approach that we are
>> trying to avoid.
>>
>> The TLMM may or not may not be powered off (depending on the SoC state)
>> and Linux has no control on it. The PDC will remain powered on but we
>> don't want the PDC interrupt enabled always, since we will receive to
>> interrupts (one from TLMM and one from PDC) for every event. So we have
>> to keep the PDC interrupt configured as wakeup interrupt but operate on
>> the fact that when we go into suspend or cpuidle we will have to switch
>> to enabling the PDC interrupt and disabling the GPIO IRQ and swap back
>> when we resume. This dance is harder with cpuidle (notifying the TLMM
>> driver, when all the CPUs are in idle) than suspend/resume which has
>> nice callbacks and is what we are trying to avoid but using the PDC
>> interrupt always.
>
>It looks to me that the way this logic is split across two drivers is
>a major cause of headache. My advise is that either you have one
>single point of interrupt handling for such interrupt, or you force a
>TLMM wake-up on such an event, forcing it to handle the interrupt the
>"normal way"...
>
The PDC irqchip's registers are the same ones used over here as well to
configure the wakeup interrupt. So locking acrossing the pinctrl and
irqchip would be annoying, but doable. Would like to avoid it if
possible.

Thanks,
Lina

Re: [PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Stephen Boyd]

Hi Lina,

Quoting Lina Iyer (2018-10-10 17:29:58)
> QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on
> domain can wakeup the SoC, when interrupts and GPIOs are routed to its
> interrupt controller. Only select GPIOs that are deemed wakeup capable
> are routed to specific PDC pins. During low power state, the pinmux
> interrupt controller may be non-functional but the PDC would be. The PDC
> can detect the wakeup GPIO is triggered and bring the TLMM to an
> operational state.
> 
> Interrupts that are level triggered will be detected at the TLMM when
> the controller becomes operational. Edge interrupts however need to be
> replayed again.
> 
> Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ,
> but keep it disabled. During suspend, we can enable the PDC IRQ instead
> of the GPIO IRQ, which may or not be detected.
> 
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---

I spoke with Marc about this at ELCE last week and I think we came up
with a plan for how to make this all work while keeping the TLMM driver
largely unaware of what's happening. One important point that we need to
keep in mind is that we don't want the interrupt to appear twice at the
GIC for the same TLMM irq, once in TLMM and once in PDC. That situation
would be quite bad and confuse things.

So the plan is as follows:

 1) Split PDC irqdomain into two domains. One for GIC interrupts and one
    for TLMM interrupts

 2) Support hierarchical irq domains in the TLMM driver and/or gpiolib

 3) Stack the irq controllers into a hierarchy so that GIC is the parent
    of PDC and PDC is the parent of TLMM while also leaving the TLMM
    summary IRQ chained directly from the GIC irqdomain

 4) Mask a TLMM irq in the TLMM driver and unmask the PDC irq in the PDC
    driver when an irq_set_wake() call is made on a TLMM gpio irq that
    PDC can monitor (and vice versa)

 5) Have the PDC driver be the only driver that knows if a TLMM pin is
    wakeup capable or not expressed in some DT table

Implementing #4 will require changing TLMM irqchip to call
irq_chip_set_wake_parent() for the .irq_set_wake op and then checking
the return value to figure out if PDC is monitoring the pin or not and
doing the mask/unmask dance. Furthermore, implementing #2 will be a bit
of work that Thierry has already started for Nvidia Tegra pinctrl
drivers.

The important part of #4 is that the irq can only happen in TLMM or in
PDC, and not in both places because of how we mask and unmask in
different drivers. The TLMM summary irq line can't be triggered when the
PDC is monitoring the line.

This also limits the usage of PDC to pins that are really waking up the
system from some sort of sleep mode. Either the device driver is trying
to wakeup from suspend so it's setting irq wake in the driver suspend
path, or it's runtime suspending a device and wanting to wakeup from
that device suspend state so it's again setting irq wake in runtime
suspend. Either way, in "normal" active modes the irq path will be
through the TLMM summary irq line and not through PDC. I don't know if
a similar design would work for pre-PDC qcom hardware where the MPM is
handled by RPM software.

And finally, thinking about this after writing this mail I'm a little
concerned that doing any sort of mask/unmask or irq movement from TLMM
to PDC at runtime will miss edges for edge triggered interrupts. Is that
also your concern? How is this handled with MPM? We would leave the TLMM
interrupt enabled in that case and then have to replay the edge in
software when resuming from suspend or idle paths but otherwise ignore
the level type interrupts that MPM sees? I suppose MPM never gets an
edge if TLMM gets the edge so this just works.

I'm worried that when we're moving the gpio irq line from the summary
GIC SPI line to the PDC's GIC SPI line we'll get an edge and then miss
it in TLMM and be too late unmasking it in PDC or worse see it in PDC
and TLMM because we unmask in PDC before masking in TLMM. So we may need
to change #4 up above to always allocate the irq from PDC and somehow
communicate that the irq is wakeup capable in PDC back to the TLMM
driver so TLMM knows to keep the irq masked in the hardware forever.
That way it can't cause the summary irq line to trigger in addition to
the PDC one. Given that we have allocation hooks with domain hierarchy
it may be easy enough to remove TLMM irqs from the summary irq domain
when they can be allocated from the parent PDC domain.

Re: [PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Lina Iyer]

On Wed, Oct 31 2018 at 01:05 -0600, Stephen Boyd wrote:
>Hi Lina,
>
>Quoting Lina Iyer (2018-10-10 17:29:58)
>> QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on
>> domain can wakeup the SoC, when interrupts and GPIOs are routed to its
>> interrupt controller. Only select GPIOs that are deemed wakeup capable
>> are routed to specific PDC pins. During low power state, the pinmux
>> interrupt controller may be non-functional but the PDC would be. The PDC
>> can detect the wakeup GPIO is triggered and bring the TLMM to an
>> operational state.
>>
>> Interrupts that are level triggered will be detected at the TLMM when
>> the controller becomes operational. Edge interrupts however need to be
>> replayed again.
>>
>> Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ,
>> but keep it disabled. During suspend, we can enable the PDC IRQ instead
>> of the GPIO IRQ, which may or not be detected.
>>
I should have removed this paragraph. This is not relevant to the
$SUBJECT patch anymore.

>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>
> So we may need
>to change #4 up above to always allocate the irq from PDC and somehow
>communicate that the irq is wakeup capable in PDC back to the TLMM
>driver so TLMM knows to keep the irq masked in the hardware forever.
>That way it can't cause the summary irq line to trigger in addition to
>the PDC one. Given that we have allocation hooks with domain hierarchy
>it may be easy enough to remove TLMM irqs from the summary irq domain
>when they can be allocated from the parent PDC domain.
>
This is exactly what this patch does. We dont want to use the GPIO IRQ
and the summary line and instead always use the PDC for all wakeup
capable GPIO IRQs. The configuration of the IRQ registers is avoided
here and therefore the TLMM never triggers the summary line at the GIC
for GPIOs that are specified in the DT. PDC is the only way to detect
the GPIO interrupt whether the system is active or not. That way we
avoid any mishaps in handshakes between the two interrupts.

Thanks,
Lina

Re: [PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Lina Iyer]

On Wed, Oct 31 2018 at 01:05 -0600, Stephen Boyd wrote:
>Hi Lina,
>
>Quoting Lina Iyer (2018-10-10 17:29:58)
>> QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on
>> domain can wakeup the SoC, when interrupts and GPIOs are routed to its
>> interrupt controller. Only select GPIOs that are deemed wakeup capable
>> are routed to specific PDC pins. During low power state, the pinmux
>> interrupt controller may be non-functional but the PDC would be. The PDC
>> can detect the wakeup GPIO is triggered and bring the TLMM to an
>> operational state.
>>
>> Interrupts that are level triggered will be detected at the TLMM when
>> the controller becomes operational. Edge interrupts however need to be
>> replayed again.
>>
>> Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ,
>> but keep it disabled. During suspend, we can enable the PDC IRQ instead
>> of the GPIO IRQ, which may or not be detected.
>>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>
>I spoke with Marc about this at ELCE last week and I think we came up
>with a plan for how to make this all work while keeping the TLMM driver
>largely unaware of what's happening. One important point that we need to
>keep in mind is that we don't want the interrupt to appear twice at the
>GIC for the same TLMM irq, once in TLMM and once in PDC. That situation
>would be quite bad and confuse things.
>
>So the plan is as follows:
>
> 1) Split PDC irqdomain into two domains. One for GIC interrupts and one
>    for TLMM interrupts
>
> 2) Support hierarchical irq domains in the TLMM driver and/or gpiolib
>
> 3) Stack the irq controllers into a hierarchy so that GIC is the parent
>    of PDC and PDC is the parent of TLMM while also leaving the TLMM
>    summary IRQ chained directly from the GIC irqdomain
>
> 4) Mask a TLMM irq in the TLMM driver and unmask the PDC irq in the PDC
>    driver when an irq_set_wake() call is made on a TLMM gpio irq that
>    PDC can monitor (and vice versa)
>
> 5) Have the PDC driver be the only driver that knows if a TLMM pin is
>    wakeup capable or not expressed in some DT table
>
>Implementing #4 will require changing TLMM irqchip to call
>irq_chip_set_wake_parent() for the .irq_set_wake op and then checking
>the return value to figure out if PDC is monitoring the pin or not and
>doing the mask/unmask dance. Furthermore, implementing #2 will be a bit
>of work that Thierry has already started for Nvidia Tegra pinctrl
>drivers.
>
We have been doing something similar downstream. But it has been quite
lot of code. The approach was to treat PDC-GPIO as a separate as
interrupt controller whose parent is the PDC. GPIOs that have PDC lines,
will be handled by the PDC-GPIO, while other GPIOs will continue to use
summary line. The hierarchy helps with the masking/unmasking and
prevents PDC interrupt from being detected as a separate interrupt than
the TLMM.

>The important part of #4 is that the irq can only happen in TLMM or in
>PDC, and not in both places because of how we mask and unmask in
>different drivers. The TLMM summary irq line can't be triggered when the
>PDC is monitoring the line.
>
>This also limits the usage of PDC to pins that are really waking up the
>system from some sort of sleep mode. Either the device driver is trying
>to wakeup from suspend so it's setting irq wake in the driver suspend
>path, or it's runtime suspending a device and wanting to wakeup from
>that device suspend state so it's again setting irq wake in runtime
>suspend. Either way, in "normal" active modes the irq path will be
>through the TLMM summary irq line and not through PDC. I don't know if
>a similar design would work for pre-PDC qcom hardware where the MPM is
>handled by RPM software.
>
Yes, we need a solution that works with both. But we can be assured that
the architecture will have either MPM or PDC, never both. The PDC is a
parallel line that is always active, while the MPM line is enabled only
when the SoC is in sleep mode.

>And finally, thinking about this after writing this mail I'm a little
>concerned that doing any sort of mask/unmask or irq movement from TLMM
>to PDC at runtime will miss edges for edge triggered interrupts. Is that
>also your concern? How is this handled with MPM? We would leave the TLMM
>interrupt enabled in that case and then have to replay the edge in
>software when resuming from suspend or idle paths but otherwise ignore
>the level type interrupts that MPM sees? I suppose MPM never gets an
>edge if TLMM gets the edge so this just works.
>
This is partly correct. Let me explain the key differences between the
current and the previous generation of the hardware to help with that.
(Let's call them PDC-based and MPM-based architectures respectively).

In MPM-based solution, the TLMM is active and is solely responsible for
notifying the CPU of the GPIOs until powered off (as part of
idle/suspend). At which point, the always-on MPM is configured for
wakeup from the GPIO. Upon sensing a signal will wake up the application
CPU using a separate MPM interrupt. The TLMM is powered on at the same
time. The CPU is interrupted by the MPM and if the GPIO was a level
interrupt the TLMM summary line will handle the GPIO. If the GPIO was an
edge interrupt the Linux MPM driver will replay the interrupt at TLMM.
Note that the MPM is not active after sending the interrupt to wakeup
the CPU.

With PDC-based solutions, the PDC is always active and can be used to
detect the interrupt even when the TLMM is active. But it could be a
problem if both the interrupt lines are configured. If we could use the
PDC interrupt line always for such GPIOs, it will solve the problem.
This patch does just that.

By using the PDC line for the PDC-based architectures, we will not
intersect the MPM-based architectures and they both can continue to use
the same driver. Only in the PDC-based architecture we will find a map
of the PDC interrupts in DT for the corresponding GPIOs. If the map is
missing or the PDC is not available, it will not be a error.

Stephen, I answered the last para in a separate email.

Thanks,
Lina

Re: [PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Stephen Boyd]

Quoting Lina Iyer (2018-10-31 09:46:50)
> On Wed, Oct 31 2018 at 01:05 -0600, Stephen Boyd wrote:
> >Hi Lina,
> >
> >Quoting Lina Iyer (2018-10-10 17:29:58)
> >> QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on
> >> domain can wakeup the SoC, when interrupts and GPIOs are routed to its
> >> interrupt controller. Only select GPIOs that are deemed wakeup capable
> >> are routed to specific PDC pins. During low power state, the pinmux
> >> interrupt controller may be non-functional but the PDC would be. The PDC
> >> can detect the wakeup GPIO is triggered and bring the TLMM to an
> >> operational state.
> >>
> >> Interrupts that are level triggered will be detected at the TLMM when
> >> the controller becomes operational. Edge interrupts however need to be
> >> replayed again.
> >>
> >> Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ,
> >> but keep it disabled. During suspend, we can enable the PDC IRQ instead
> >> of the GPIO IRQ, which may or not be detected.
> >>
> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> >> ---
> >
> >I spoke with Marc about this at ELCE last week and I think we came up
> >with a plan for how to make this all work while keeping the TLMM driver
> >largely unaware of what's happening. One important point that we need to
> >keep in mind is that we don't want the interrupt to appear twice at the
> >GIC for the same TLMM irq, once in TLMM and once in PDC. That situation
> >would be quite bad and confuse things.
> >
> >So the plan is as follows:
> >
> > 1) Split PDC irqdomain into two domains. One for GIC interrupts and one
> >    for TLMM interrupts
> >
> > 2) Support hierarchical irq domains in the TLMM driver and/or gpiolib
> >
> > 3) Stack the irq controllers into a hierarchy so that GIC is the parent
> >    of PDC and PDC is the parent of TLMM while also leaving the TLMM
> >    summary IRQ chained directly from the GIC irqdomain
> >
> > 4) Mask a TLMM irq in the TLMM driver and unmask the PDC irq in the PDC
> >    driver when an irq_set_wake() call is made on a TLMM gpio irq that
> >    PDC can monitor (and vice versa)
> >
> > 5) Have the PDC driver be the only driver that knows if a TLMM pin is
> >    wakeup capable or not expressed in some DT table
> >
> >Implementing #4 will require changing TLMM irqchip to call
> >irq_chip_set_wake_parent() for the .irq_set_wake op and then checking
> >the return value to figure out if PDC is monitoring the pin or not and
> >doing the mask/unmask dance. Furthermore, implementing #2 will be a bit
> >of work that Thierry has already started for Nvidia Tegra pinctrl
> >drivers.
> >
> We have been doing something similar downstream. But it has been quite
> lot of code. The approach was to treat PDC-GPIO as a separate as
> interrupt controller whose parent is the PDC. GPIOs that have PDC lines,
> will be handled by the PDC-GPIO, while other GPIOs will continue to use
> summary line. The hierarchy helps with the masking/unmasking and
> prevents PDC interrupt from being detected as a separate interrupt than
> the TLMM.
> 
> >The important part of #4 is that the irq can only happen in TLMM or in
> >PDC, and not in both places because of how we mask and unmask in
> >different drivers. The TLMM summary irq line can't be triggered when the
> >PDC is monitoring the line.
> >
> >This also limits the usage of PDC to pins that are really waking up the
> >system from some sort of sleep mode. Either the device driver is trying
> >to wakeup from suspend so it's setting irq wake in the driver suspend
> >path, or it's runtime suspending a device and wanting to wakeup from
> >that device suspend state so it's again setting irq wake in runtime
> >suspend. Either way, in "normal" active modes the irq path will be
> >through the TLMM summary irq line and not through PDC. I don't know if
> >a similar design would work for pre-PDC qcom hardware where the MPM is
> >handled by RPM software.
> >
> Yes, we need a solution that works with both. But we can be assured that
> the architecture will have either MPM or PDC, never both. The PDC is a
> parallel line that is always active, while the MPM line is enabled only
> when the SoC is in sleep mode.
> 
> >And finally, thinking about this after writing this mail I'm a little
> >concerned that doing any sort of mask/unmask or irq movement from TLMM
> >to PDC at runtime will miss edges for edge triggered interrupts. Is that
> >also your concern? How is this handled with MPM? We would leave the TLMM
> >interrupt enabled in that case and then have to replay the edge in
> >software when resuming from suspend or idle paths but otherwise ignore
> >the level type interrupts that MPM sees? I suppose MPM never gets an
> >edge if TLMM gets the edge so this just works.
> >
> This is partly correct. Let me explain the key differences between the
> current and the previous generation of the hardware to help with that.
> (Let's call them PDC-based and MPM-based architectures respectively).
> 
> In MPM-based solution, the TLMM is active and is solely responsible for
> notifying the CPU of the GPIOs until powered off (as part of
> idle/suspend). At which point, the always-on MPM is configured for
> wakeup from the GPIO. Upon sensing a signal will wake up the application
> CPU using a separate MPM interrupt. The TLMM is powered on at the same
> time. The CPU is interrupted by the MPM and if the GPIO was a level
> interrupt the TLMM summary line will handle the GPIO. If the GPIO was an
> edge interrupt the Linux MPM driver will replay the interrupt at TLMM.
> Note that the MPM is not active after sending the interrupt to wakeup
> the CPU.
> 
> With PDC-based solutions, the PDC is always active and can be used to
> detect the interrupt even when the TLMM is active. But it could be a
> problem if both the interrupt lines are configured. If we could use the
> PDC interrupt line always for such GPIOs, it will solve the problem.
> This patch does just that.

Right. Let's scrap the plan to do the wakeup based mask/unmask in both
chips. It won't work because of the edge trigger type.

The difference I see is that this patch does the irq "forwarding" by
making the TLMM driver highly aware of the PDC irq domain. It explicitly
lists each PDC irq as interrupts in DT. Why are we doing that? If we
used hierarchical irq domains then only the PDC would be aware of what
irqs are wakeup capable, and TLMM would just need to be told to mask or
not mask the irq when an irq is monitored by the PDC (and maybe not even
that).

This is also the only major difference I see between MPM and PDC based
designs. In the PDC case, we need to mask the irq in TLMM when PDC can
monitor it, while in the MPM case we want to keep it unmasked in TLMM
all the time and only have MPM configure the wakeup on irq_set_wake().
In the end, supporting MPM is simpler because it sits in-between TLMM
and GIC, watches all TLMM irqs get allocated and hooks the irq_set_wake
calls for the ones that it cares to wakeup with and masks the non-wakeup
irqs during suspend, and then does edge trigger replays when the MPM
interrupt handler runs by poking the TLMM hardware directly. That poke
causes the summary irq line to go high and the GIC SPI line for TLMM
summary services the GPIO irq. We would leave the level type irqs alone
because hardware takes care of it all for us.

Can PDC be made to do the same thing as MPM? On resume we can replay any
pending irq that's edge triggered by replaying the edge into the TLMM.
That will cause the summary irq line at the GIC to go high and then the
chained irq handler will run as normal. As long as we don't need to
replay edges during deep idle states I think this can work, and given
that MPM isn't replaying edges during deep idle I suspect this is all
fine. Yes we have a GIC SPI line for each pin that PDC can monitor, but
that's largely an implementation detail here.

It would be nice to make MPM and PDC the same, so that we don't need to
decide to mask or not mask in TLMM or move a GPIO irq out of the TLMM
summary GIC SPI to some other GIC SPI from PDC. Then supporting both
designs is nothing more than parenting the MPM or PDC to TLMM and having
those drivers poke the edges into the hardware on resume.

> 
> By using the PDC line for the PDC-based architectures, we will not
> intersect the MPM-based architectures and they both can continue to use
> the same driver. Only in the PDC-based architecture we will find a map
> of the PDC interrupts in DT for the corresponding GPIOs. If the map is
> missing or the PDC is not available, it will not be a error.
> 

Well given that we don't have any MPM based solutions upstream I'm not
sure how well we can make sure both designs work together, but yes, we
should strive to do this in case MPM solutions get upstreamed.

Re: [PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Lina Iyer]

On Wed, Oct 31 2018 at 18:13 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2018-10-31 09:46:50)
>> On Wed, Oct 31 2018 at 01:05 -0600, Stephen Boyd wrote:
>> >Hi Lina,
>> >
>> >Quoting Lina Iyer (2018-10-10 17:29:58)
>> >> QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on
>> >> domain can wakeup the SoC, when interrupts and GPIOs are routed to its
>> >> interrupt controller. Only select GPIOs that are deemed wakeup capable
>> >> are routed to specific PDC pins. During low power state, the pinmux
>> >> interrupt controller may be non-functional but the PDC would be. The PDC
>> >> can detect the wakeup GPIO is triggered and bring the TLMM to an
>> >> operational state.
>> >>
>> >> Interrupts that are level triggered will be detected at the TLMM when
>> >> the controller becomes operational. Edge interrupts however need to be
>> >> replayed again.
>> >>
>> >> Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ,
>> >> but keep it disabled. During suspend, we can enable the PDC IRQ instead
>> >> of the GPIO IRQ, which may or not be detected.
>> >>
>> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> >> ---
>> >
>> >I spoke with Marc about this at ELCE last week and I think we came up
>> >with a plan for how to make this all work while keeping the TLMM driver
>> >largely unaware of what's happening. One important point that we need to
>> >keep in mind is that we don't want the interrupt to appear twice at the
>> >GIC for the same TLMM irq, once in TLMM and once in PDC. That situation
>> >would be quite bad and confuse things.
>> >
>> >So the plan is as follows:
>> >
>> > 1) Split PDC irqdomain into two domains. One for GIC interrupts and one
>> >    for TLMM interrupts
>> >
>> > 2) Support hierarchical irq domains in the TLMM driver and/or gpiolib
>> >
>> > 3) Stack the irq controllers into a hierarchy so that GIC is the parent
>> >    of PDC and PDC is the parent of TLMM while also leaving the TLMM
>> >    summary IRQ chained directly from the GIC irqdomain
>> >
>> > 4) Mask a TLMM irq in the TLMM driver and unmask the PDC irq in the PDC
>> >    driver when an irq_set_wake() call is made on a TLMM gpio irq that
>> >    PDC can monitor (and vice versa)
>> >
>> > 5) Have the PDC driver be the only driver that knows if a TLMM pin is
>> >    wakeup capable or not expressed in some DT table
>> >
>> >Implementing #4 will require changing TLMM irqchip to call
>> >irq_chip_set_wake_parent() for the .irq_set_wake op and then checking
>> >the return value to figure out if PDC is monitoring the pin or not and
>> >doing the mask/unmask dance. Furthermore, implementing #2 will be a bit
>> >of work that Thierry has already started for Nvidia Tegra pinctrl
>> >drivers.
>> >
>> We have been doing something similar downstream. But it has been quite
>> lot of code. The approach was to treat PDC-GPIO as a separate as
>> interrupt controller whose parent is the PDC. GPIOs that have PDC lines,
>> will be handled by the PDC-GPIO, while other GPIOs will continue to use
>> summary line. The hierarchy helps with the masking/unmasking and
>> prevents PDC interrupt from being detected as a separate interrupt than
>> the TLMM.
>>
>> >The important part of #4 is that the irq can only happen in TLMM or in
>> >PDC, and not in both places because of how we mask and unmask in
>> >different drivers. The TLMM summary irq line can't be triggered when the
>> >PDC is monitoring the line.
>> >
>> >This also limits the usage of PDC to pins that are really waking up the
>> >system from some sort of sleep mode. Either the device driver is trying
>> >to wakeup from suspend so it's setting irq wake in the driver suspend
>> >path, or it's runtime suspending a device and wanting to wakeup from
>> >that device suspend state so it's again setting irq wake in runtime
>> >suspend. Either way, in "normal" active modes the irq path will be
>> >through the TLMM summary irq line and not through PDC. I don't know if
>> >a similar design would work for pre-PDC qcom hardware where the MPM is
>> >handled by RPM software.
>> >
>> Yes, we need a solution that works with both. But we can be assured that
>> the architecture will have either MPM or PDC, never both. The PDC is a
>> parallel line that is always active, while the MPM line is enabled only
>> when the SoC is in sleep mode.
>>
>> >And finally, thinking about this after writing this mail I'm a little
>> >concerned that doing any sort of mask/unmask or irq movement from TLMM
>> >to PDC at runtime will miss edges for edge triggered interrupts. Is that
>> >also your concern? How is this handled with MPM? We would leave the TLMM
>> >interrupt enabled in that case and then have to replay the edge in
>> >software when resuming from suspend or idle paths but otherwise ignore
>> >the level type interrupts that MPM sees? I suppose MPM never gets an
>> >edge if TLMM gets the edge so this just works.
>> >
>> This is partly correct. Let me explain the key differences between the
>> current and the previous generation of the hardware to help with that.
>> (Let's call them PDC-based and MPM-based architectures respectively).
>>
>> In MPM-based solution, the TLMM is active and is solely responsible for
>> notifying the CPU of the GPIOs until powered off (as part of
>> idle/suspend). At which point, the always-on MPM is configured for
>> wakeup from the GPIO. Upon sensing a signal will wake up the application
>> CPU using a separate MPM interrupt. The TLMM is powered on at the same
>> time. The CPU is interrupted by the MPM and if the GPIO was a level
>> interrupt the TLMM summary line will handle the GPIO. If the GPIO was an
>> edge interrupt the Linux MPM driver will replay the interrupt at TLMM.
>> Note that the MPM is not active after sending the interrupt to wakeup
>> the CPU.
>>
>> With PDC-based solutions, the PDC is always active and can be used to
>> detect the interrupt even when the TLMM is active. But it could be a
>> problem if both the interrupt lines are configured. If we could use the
>> PDC interrupt line always for such GPIOs, it will solve the problem.
>> This patch does just that.
>
>Right. Let's scrap the plan to do the wakeup based mask/unmask in both
>chips. It won't work because of the edge trigger type.
>
>The difference I see is that this patch does the irq "forwarding" by
>making the TLMM driver highly aware of the PDC irq domain. It explicitly
>lists each PDC irq as interrupts in DT. Why are we doing that?
The DT interrupts are a way of mapping the GPIO to their corresponding
PDC lines.

>If we used hierarchical irq domains then only the PDC would be aware of
>what irqs are wakeup capable, and TLMM would just need to be told to
>mask or not mask the irq when an irq is monitored by the PDC (and maybe
>not even that).
>
The reason, why I didn't use hierarchical irq domains is that not all
GPIO interrupts are routed to the PDC and the summary line doesn't go
into the PDC.

>This is also the only major difference I see between MPM and PDC based
>designs. In the PDC case, we need to mask the irq in TLMM when PDC can
>monitor it, while in the MPM case we want to keep it unmasked in TLMM
>all the time and only have MPM configure the wakeup on irq_set_wake().
>
In MPM based solutions, there is a clear handover between when MPM takes
over and when TLMM is responsible for the GPIO interrupt. It is not the
case with PDC. The PDC is always on and monitoring. We dont know when
the TLMM will be rendered incapable of sensing interrupt. To avoid
sensing two interrupts you want only one of the two active. That is the
tricky part, to know when to switch over.

>In the end, supporting MPM is simpler because it sits in-between TLMM
>and GIC, watches all TLMM irqs get allocated and hooks the irq_set_wake
>calls for the ones that it cares to wakeup with and masks the non-wakeup
>irqs during suspend, and then does edge trigger replays when the MPM
>interrupt handler runs by poking the TLMM hardware directly. That poke
>causes the summary irq line to go high and the GIC SPI line for TLMM
>summary services the GPIO irq. We would leave the level type irqs alone
>because hardware takes care of it all for us.
>
Yes, but more importantly, the remote processor knows when to switch
over to MPM and knows if we are going to lose TLMM's interrupt sensing
capability. With PDC there is no central entity to do that.

>Can PDC be made to do the same thing as MPM? On resume we can replay any
>pending irq that's edge triggered by replaying the edge into the TLMM.
>That will cause the summary irq line at the GIC to go high and then the
>chained irq handler will run as normal. As long as we don't need to
>replay edges during deep idle states I think this can work, and given
>that MPM isn't replaying edges during deep idle I suspect this is all
>fine.
>
MPM replays the edges during deep idle. There is no difference between
deep idle and suspend/resume. If the devices are not in use, we would go
into a idle state that would disable the TLMM's interrupt controller and
when we come out of idle, we will replay the wakeup interrupt in the MPM
driver.

>Yes we have a GIC SPI line for each pin that PDC can monitor, but
>that's largely an implementation detail here.
>
>It would be nice to make MPM and PDC the same, so that we don't need to
>decide to mask or not mask in TLMM or move a GPIO irq out of the TLMM
>summary GIC SPI to some other GIC SPI from PDC. Then supporting both
>designs is nothing more than parenting the MPM or PDC to TLMM and having
>those drivers poke the edges into the hardware on resume.
>
Agreed. That should the do-able.

-- Lina

Re: [PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Stephen Boyd]

Quoting Lina Iyer (2018-11-01 10:16:30)
> On Wed, Oct 31 2018 at 18:13 -0600, Stephen Boyd wrote:
> >
> >Right. Let's scrap the plan to do the wakeup based mask/unmask in both
> >chips. It won't work because of the edge trigger type.
> >
> >The difference I see is that this patch does the irq "forwarding" by
> >making the TLMM driver highly aware of the PDC irq domain. It explicitly
> >lists each PDC irq as interrupts in DT. Why are we doing that?
> The DT interrupts are a way of mapping the GPIO to their corresponding
> PDC lines.

Agreed. One problem I see with this approach is that PDC interrupts will
have stats and affinity associated with them and that won't be the same
as the real GPIO interrupt which will also have stats and affinity, etc.

> 
> >If we used hierarchical irq domains then only the PDC would be aware of
> >what irqs are wakeup capable, and TLMM would just need to be told to
> >mask or not mask the irq when an irq is monitored by the PDC (and maybe
> >not even that).
> >
> The reason, why I didn't use hierarchical irq domains is that not all
> GPIO interrupts are routed to the PDC and the summary line doesn't go
> into the PDC.

Ok. I don't see why we need to limit ourselves here. If a GPIO interrupt
isn't routed through PDC physically why does it matter? It shouldn't be
why we avoid hierarchical irq domains.

> 
> >This is also the only major difference I see between MPM and PDC based
> >designs. In the PDC case, we need to mask the irq in TLMM when PDC can
> >monitor it, while in the MPM case we want to keep it unmasked in TLMM
> >all the time and only have MPM configure the wakeup on irq_set_wake().
> >
> In MPM based solutions, there is a clear handover between when MPM takes
> over and when TLMM is responsible for the GPIO interrupt. It is not the
> case with PDC. The PDC is always on and monitoring. We dont know when
> the TLMM will be rendered incapable of sensing interrupt. To avoid
> sensing two interrupts you want only one of the two active. That is the
> tricky part, to know when to switch over.

Alright, well then we need to never switch over while the irq is
active/requested.

> 
> >In the end, supporting MPM is simpler because it sits in-between TLMM
> >and GIC, watches all TLMM irqs get allocated and hooks the irq_set_wake
> >calls for the ones that it cares to wakeup with and masks the non-wakeup
> >irqs during suspend, and then does edge trigger replays when the MPM
> >interrupt handler runs by poking the TLMM hardware directly. That poke
> >causes the summary irq line to go high and the GIC SPI line for TLMM
> >summary services the GPIO irq. We would leave the level type irqs alone
> >because hardware takes care of it all for us.
> >
> Yes, but more importantly, the remote processor knows when to switch
> over to MPM and knows if we are going to lose TLMM's interrupt sensing
> capability. With PDC there is no central entity to do that.

Ok.

> 
> >Can PDC be made to do the same thing as MPM? On resume we can replay any
> >pending irq that's edge triggered by replaying the edge into the TLMM.
> >That will cause the summary irq line at the GIC to go high and then the
> >chained irq handler will run as normal. As long as we don't need to
> >replay edges during deep idle states I think this can work, and given
> >that MPM isn't replaying edges during deep idle I suspect this is all
> >fine.
> >
> MPM replays the edges during deep idle. There is no difference between
> deep idle and suspend/resume. If the devices are not in use, we would go
> into a idle state that would disable the TLMM's interrupt controller and
> when we come out of idle, we will replay the wakeup interrupt in the MPM
> driver.

Ah ok, that sort of makes having PDC act like MPM not work because
replaying from deep idle isn't going to work. I was relying on cpu
suspend state where only one CPU is online and irqs are disabled to be
the time when irqs are replayed. It's probably slower that way too on
PDC because we have to bounce through the replay and have another
register write for each GPIO interrupt.

> 
> >Yes we have a GIC SPI line for each pin that PDC can monitor, but
> >that's largely an implementation detail here.
> >
> >It would be nice to make MPM and PDC the same, so that we don't need to
> >decide to mask or not mask in TLMM or move a GPIO irq out of the TLMM
> >summary GIC SPI to some other GIC SPI from PDC. Then supporting both
> >designs is nothing more than parenting the MPM or PDC to TLMM and having
> >those drivers poke the edges into the hardware on resume.
> >
> Agreed. That should the do-able.
> 

It doesn't seem like they can be exactly the same because of deep idle
wakeup, so I suggest we go back to the beginning:

 * Split PDC into two domains, one for GIC and one for TLMM and
   associate the TLMM one with an irq_domain_bus_token enum so
   that TLMM can look it up with irq_find_matching_host()

 * Set TLMM as the child of the PDC TLMM domain

 * Allocate irqs from TLMM and have those allocate in the parent domain
   if the TLMM is in a hierarchy domain with irq_domain_alloc_irqs()

 * When allocating an irq in TLMM's irqdomain::alloc function pass a
   TLMM specific struct to irq_domain_alloc_irqs_parent() last argument

   struct qcom_tlmm_fwspec {
      bool mask;
      struct irq_fwspec spec;
   };
   
 * Have the PDC driver set the mask bit and the MPM driver not set the
   mask bit in the qcom_tlmm_fwspec above

 * Have the TLMM driver use the mask bit to figure out if it should mask
   or not mask the interrupt in the TLMM hardware

This way we can use the last argument to irq_domain_alloc_irqs_parent()
to communicate between the PDC/MPM drivers and the TLMM driver about how
the irq is being managed by the wakeup domain.

There will be some other functions to call for the hierarchy irq calls
in the TLMM code, specifically the set_type callback and the set_wake
callback need to call up to the parent irq domain so the MPM can trap
the set_wake calls to know what to monitor and the PDC can trigger
correctly. You can look at what Thierry has done for Tegra[1] and copy a
lot of that patch series.

The main difference is that on Tegra they have hardware replay the edge
vs. on qcom platforms we'll rely on the GIC SPI line for each PDC line
to go high and then have the hierarchy replay the interrupt. Also, on
Tegra they don't really care to allocate the irq in the GIC, whereas on
qcom we'll want to allocate the interrupt in the GIC to get the replay
to work. In the MPM design I think we'll need to use the
irq_set_irqchip_state() API too so the MPM driver can replay the edge
interrupts into the TLMM hardware when the MPM summary irq runs.

[1] https://lkml.org/lkml/2018/9/21/471

Re: [PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Lina Iyer]

On Mon, Nov 05 2018 at 22:19 -0700, Stephen Boyd wrote:
>Quoting Lina Iyer (2018-11-01 10:16:30)
>> On Wed, Oct 31 2018 at 18:13 -0600, Stephen Boyd wrote:
>> >
[...]

>Ok. I don't see why we need to limit ourselves here. If a GPIO interrupt
>isn't routed through PDC physically why does it matter? It shouldn't be
>why we avoid hierarchical irq domains.
>
Ok, now that we are not limited by this requirement...

>It doesn't seem like they can be exactly the same because of deep idle
>wakeup, so I suggest we go back to the beginning:
>
> * Split PDC into two domains, one for GIC and one for TLMM and
>   associate the TLMM one with an irq_domain_bus_token enum so
>   that TLMM can look it up with irq_find_matching_host()
>
The complexity here is that the GPIO and GIC interrupts are intertwined
at the PDC.
It would be ugly to describe the GIC interrupts with holes for the GPIOs
and the other way around as well. A single PDC instance would be best
way to keep things simple and clean.

Instead, we could have TLMM -> TLMM-PDC -> PDC -> GIC. The TLMM-PDC
being the interrupt controller that sets up the IRQ parent for the GPIO
IRQs.

Thanks,
Lina

> * Set TLMM as the child of the PDC TLMM domain
>
> * Allocate irqs from TLMM and have those allocate in the parent domain
>   if the TLMM is in a hierarchy domain with irq_domain_alloc_irqs()
>
> * When allocating an irq in TLMM's irqdomain::alloc function pass a
>   TLMM specific struct to irq_domain_alloc_irqs_parent() last argument
>
>   struct qcom_tlmm_fwspec {
>      bool mask;
>      struct irq_fwspec spec;
>   };
>
> * Have the PDC driver set the mask bit and the MPM driver not set the
>   mask bit in the qcom_tlmm_fwspec above
>
> * Have the TLMM driver use the mask bit to figure out if it should mask
>   or not mask the interrupt in the TLMM hardware
>
>This way we can use the last argument to irq_domain_alloc_irqs_parent()
>to communicate between the PDC/MPM drivers and the TLMM driver about how
>the irq is being managed by the wakeup domain.
>
>There will be some other functions to call for the hierarchy irq calls
>in the TLMM code, specifically the set_type callback and the set_wake
>callback need to call up to the parent irq domain so the MPM can trap
>the set_wake calls to know what to monitor and the PDC can trigger
>correctly. You can look at what Thierry has done for Tegra[1] and copy a
>lot of that patch series.
>
>The main difference is that on Tegra they have hardware replay the edge
>vs. on qcom platforms we'll rely on the GIC SPI line for each PDC line
>to go high and then have the hierarchy replay the interrupt. Also, on
>Tegra they don't really care to allocate the irq in the GIC, whereas on
>qcom we'll want to allocate the interrupt in the GIC to get the replay
>to work. In the MPM design I think we'll need to use the
>irq_set_irqchip_state() API too so the MPM driver can replay the edge
>interrupts into the TLMM hardware when the MPM summary irq runs.
>
>[1] https://lkml.org/lkml/2018/9/21/471
>