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

[Marc Zyngier <marc.zyngier@arm.com>]

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