[PATCH RESEND RFC 0/4] Wakeup GPIO support for SDM845 SoC

[PATCH RESEND RFC 0/4] Wakeup GPIO support for SDM845 SoC

[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

[PATCH RESEND RFC 1/4] 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 the
its interrupt controller. Select GPIOs that are deemed wakeup capable are
routed to specific PDC pins. The PDC wakes up the GIC and replays the
interrupt at the GIC and the interrupt handler for the GPIO is invoked.

Setup the PDC IRQ when the GPIO's IRQ is requested and enable the PDC
IRQ when the GPIO's IRQ is enabled.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 163 +++++++++++++++++++++++++++++
 drivers/pinctrl/qcom/pinctrl-msm.h |  14 +++
 2 files changed, 177 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 0e22f52b2a19..39c3934712f7 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -71,6 +71,13 @@ struct msm_pinctrl {

 	const struct msm_pinctrl_soc_data *soc;
 	void __iomem *regs;
+	struct list_head pdc_irqs;
+};
+
+struct wakeup_gpio_irq_map {
+	struct list_head list;
+	unsigned gpio;
+	unsigned pdc_irq;
 };

 static int msm_get_groups_count(struct pinctrl_dev *pctldev)
@@ -558,6 +565,39 @@ static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 #define msm_gpio_dbg_show NULL
 #endif

+static int msm_gpio_get_pdc_pin(struct msm_pinctrl *pctrl, unsigned hwirq)
+{
+	struct msm_pinctrl_pdc_map *map = pctrl->soc->pdc_map;
+	int i;
+
+	for (i = 0; i < pctrl->soc->npdc_pins; i++) {
+		if (map[i].hwirq == hwirq)
+			return map[i].pdc_pin;
+	}
+
+	return -ENOTCONN;
+}
+
+static struct irq_data *msm_get_pdc_irq_data(struct irq_data *d)
+{
+	struct wakeup_gpio_irq_map *p;
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	struct irq_data *data = NULL;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	list_for_each_entry(p, &pctrl->pdc_irqs, list) {
+		if (p->gpio == d->hwirq) {
+			data = irq_get_irq_data(p->pdc_irq);
+			break;
+		}
+	}
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+
+	return data;
+}
+
 static const struct gpio_chip msm_gpio_template = {
 	.direction_input  = msm_gpio_direction_input,
 	.direction_output = msm_gpio_direction_output,
@@ -687,6 +727,11 @@ 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 = msm_get_pdc_irq_data(d);
+
+	// TODO: Lock PDC irq chip and set type?
+	if (pdc_irqd)
+		pdc_irqd->chip->irq_set_type(pdc_irqd, type);

 	g = &pctrl->soc->groups[d->hwirq];

@@ -779,9 +824,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 = msm_get_pdc_irq_data(d);

 	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);
@@ -863,6 +912,117 @@ 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_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);
+
+	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);
+	unsigned pin, npins, irq;
+	struct wakeup_gpio_irq_map *p;
+	unsigned long flags, trigger;
+	const char *pin_name;
+	int i, ret;
+
+	pin = msm_gpio_get_pdc_pin(pctrl, d->hwirq);
+	if (pin < 0)
+		return 0;
+
+	npins = platform_irq_count(pdev);
+	if (npins <= 0)
+		return npins;
+
+	for (i = 0; i < npins; i++) {
+		irq = platform_get_irq(pdev, i);
+		if (irq >= 0 && pin == irq_get_irq_data(irq)->hwirq)
+			break;
+	}
+	if (i == npins)
+		return 0;
+
+	pin_name = kasprintf(GFP_KERNEL, "gpio-%lu", d->hwirq);
+	if (!pin_name)
+		return -ENOMEM;
+
+	trigger = irqd_get_trigger_type(d) | IRQF_ONESHOT | IRQF_NO_SUSPEND;
+	ret = request_irq(irq, wake_irq_gpio_handler, trigger, pin_name, d);
+	if (ret) {
+		pr_warn("GPIO-%lu could not be set up as wakeup", d->hwirq);
+		return ret;
+	}
+
+	p = kzalloc(sizeof(p), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+
+	p->pdc_irq = irq;
+	p->gpio = d->hwirq;
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	list_add(&p->list, &pctrl->pdc_irqs);
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+
+	return 0;
+}
+
+static int msm_gpio_pdc_pin_release(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	struct wakeup_gpio_irq_map *p, *n, *t = NULL;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	list_for_each_entry_safe(p, n, &pctrl->pdc_irqs, list) {
+		if (p->gpio == d->hwirq) {
+			list_del(&p->list);
+			t = p;
+			break;
+		}
+	}
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	if (t) {
+		free_irq(t->pdc_irq, NULL);
+		kfree(t);
+	}
+
+	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;
@@ -887,6 +1047,9 @@ 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;
+	INIT_LIST_HEAD(&pctrl->pdc_irqs);

 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
index 9b9feea540ff..5b7f3160affe 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.h
+++ b/drivers/pinctrl/qcom/pinctrl-msm.h
@@ -97,6 +97,16 @@ struct msm_pingroup {
 	unsigned intr_detection_width:5;
 };

+/**
+ * struct msm_pinctrl_pdc_map - Map GPIOs to PDC pins on RPMH based SoCs
+ * @hwirq:	   The GPIO that is mapped.
+ * @pdc_pin:	   The PDC pin to with the GPIO IRQ line is routed.
+ */
+struct msm_pinctrl_pdc_map {
+	u32 hwirq;
+	u32 pdc_pin;
+};
+
 /**
  * struct msm_pinctrl_soc_data - Qualcomm pin controller driver configuration
  * @pins:	    An array describing all pins the pin controller affects.
@@ -107,6 +117,8 @@ struct msm_pingroup {
  * @ngroups:	    The numbmer of entries in @groups.
  * @ngpio:	    The number of pingroups the driver should expose as GPIOs.
  * @pull_no_keeper: The SoC does not support keeper bias.
+ * @pdc_map:	    The map of GPIOs to the always-on PDC interrupt lines.
+ * @npdc_pins:	    The number of GPIOs mapped to the PDC pins in @pdc_map.
  */
 struct msm_pinctrl_soc_data {
 	const struct pinctrl_pin_desc *pins;
@@ -117,6 +129,8 @@ struct msm_pinctrl_soc_data {
 	unsigned ngroups;
 	unsigned ngpios;
 	bool pull_no_keeper;
+	struct msm_pinctrl_pdc_map *pdc_map;
+	unsigned npdc_pins;
 };

 int msm_pinctrl_probe(struct platform_device *pdev,
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Marc Zyngier]

On Wed, 01 Aug 2018 03:00:18 +0100,
Lina Iyer <ilina@codeaurora.org> 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 the
> its interrupt controller. Select GPIOs that are deemed wakeup capable are
> routed to specific PDC pins. The PDC wakes up the GIC and replays the
> interrupt at the GIC and the interrupt handler for the GPIO is invoked.
> 
> Setup the PDC IRQ when the GPIO's IRQ is requested and enable the PDC
> IRQ when the GPIO's IRQ is enabled.
> 
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 163 +++++++++++++++++++++++++++++
>  drivers/pinctrl/qcom/pinctrl-msm.h |  14 +++
>  2 files changed, 177 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 0e22f52b2a19..39c3934712f7 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -71,6 +71,13 @@ struct msm_pinctrl {
> 
>  	const struct msm_pinctrl_soc_data *soc;
>  	void __iomem *regs;
> +	struct list_head pdc_irqs;
> +};
> +
> +struct wakeup_gpio_irq_map {
> +	struct list_head list;
> +	unsigned gpio;
> +	unsigned pdc_irq;
>  };
> 
>  static int msm_get_groups_count(struct pinctrl_dev *pctldev)
> @@ -558,6 +565,39 @@ static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
>  #define msm_gpio_dbg_show NULL
>  #endif
> 
> +static int msm_gpio_get_pdc_pin(struct msm_pinctrl *pctrl, unsigned hwirq)
> +{
> +	struct msm_pinctrl_pdc_map *map = pctrl->soc->pdc_map;
> +	int i;
> +
> +	for (i = 0; i < pctrl->soc->npdc_pins; i++) {
> +		if (map[i].hwirq == hwirq)
> +			return map[i].pdc_pin;
> +	}
> +
> +	return -ENOTCONN;
> +}
> +
> +static struct irq_data *msm_get_pdc_irq_data(struct irq_data *d)
> +{
> +	struct wakeup_gpio_irq_map *p;
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +	struct irq_data *data = NULL;
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&pctrl->lock, flags);
> +	list_for_each_entry(p, &pctrl->pdc_irqs, list) {
> +		if (p->gpio == d->hwirq) {
> +			data = irq_get_irq_data(p->pdc_irq);
> +			break;
> +		}
> +	}
> +	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	return data;
> +}

This looks ugly. See below.

> +
>  static const struct gpio_chip msm_gpio_template = {
>  	.direction_input  = msm_gpio_direction_input,
>  	.direction_output = msm_gpio_direction_output,
> @@ -687,6 +727,11 @@ 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 = msm_get_pdc_irq_data(d);
> +
> +	// TODO: Lock PDC irq chip and set type?
> +	if (pdc_irqd)
> +		pdc_irqd->chip->irq_set_type(pdc_irqd, type);
> 
>  	g = &pctrl->soc->groups[d->hwirq];
> 
> @@ -779,9 +824,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 = msm_get_pdc_irq_data(d);
> 
>  	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);
> @@ -863,6 +912,117 @@ 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_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,

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.

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?

> +
> +	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);
> +	unsigned pin, npins, irq;
> +	struct wakeup_gpio_irq_map *p;
> +	unsigned long flags, trigger;
> +	const char *pin_name;
> +	int i, ret;
> +
> +	pin = msm_gpio_get_pdc_pin(pctrl, d->hwirq);
> +	if (pin < 0)
> +		return 0;
> +
> +	npins = platform_irq_count(pdev);
> +	if (npins <= 0)
> +		return npins;
> +
> +	for (i = 0; i < npins; i++) {
> +		irq = platform_get_irq(pdev, i);
> +		if (irq >= 0 && pin == irq_get_irq_data(irq)->hwirq)
> +			break;
> +	}
> +	if (i == npins)
> +		return 0;
> +
> +	pin_name = kasprintf(GFP_KERNEL, "gpio-%lu", d->hwirq);
> +	if (!pin_name)
> +		return -ENOMEM;
> +
> +	trigger = irqd_get_trigger_type(d) | IRQF_ONESHOT | IRQF_NO_SUSPEND;
> +	ret = request_irq(irq, wake_irq_gpio_handler, trigger, pin_name, d);
> +	if (ret) {
> +		pr_warn("GPIO-%lu could not be set up as wakeup", d->hwirq);
> +		return ret;
> +	}
> +
> +	p = kzalloc(sizeof(p), GFP_KERNEL);
> +	if (!p)
> +		return -ENOMEM;
> +
> +	p->pdc_irq = irq;
> +	p->gpio = d->hwirq;
> +	raw_spin_lock_irqsave(&pctrl->lock, flags);
> +	list_add(&p->list, &pctrl->pdc_irqs);
> +	raw_spin_unlock_irqrestore(&pctrl->lock, flags);

This whole list business seems bizarre. Why don't you use the
handler_data instead?

> +
> +	return 0;
> +}
> +
> +static int msm_gpio_pdc_pin_release(struct irq_data *d)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +	struct wakeup_gpio_irq_map *p, *n, *t = NULL;
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&pctrl->lock, flags);
> +	list_for_each_entry_safe(p, n, &pctrl->pdc_irqs, list) {
> +		if (p->gpio == d->hwirq) {
> +			list_del(&p->list);
> +			t = p;
> +			break;
> +		}
> +	}
> +	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> +	if (t) {
> +		free_irq(t->pdc_irq, NULL);

NULL? This should balance with the request_irq call, I believe.

> +		kfree(t);
> +	}
> +
> +	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;
> @@ -887,6 +1047,9 @@ 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;
> +	INIT_LIST_HEAD(&pctrl->pdc_irqs);
> 
>  	ret = gpiochip_add_data(&pctrl->chip, pctrl);
>  	if (ret) {
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> index 9b9feea540ff..5b7f3160affe 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> @@ -97,6 +97,16 @@ struct msm_pingroup {
>  	unsigned intr_detection_width:5;
>  };
> 
> +/**
> + * struct msm_pinctrl_pdc_map - Map GPIOs to PDC pins on RPMH based SoCs
> + * @hwirq:	   The GPIO that is mapped.
> + * @pdc_pin:	   The PDC pin to with the GPIO IRQ line is routed.
> + */
> +struct msm_pinctrl_pdc_map {
> +	u32 hwirq;
> +	u32 pdc_pin;
> +};
> +
>  /**
>   * struct msm_pinctrl_soc_data - Qualcomm pin controller driver configuration
>   * @pins:	    An array describing all pins the pin controller affects.
> @@ -107,6 +117,8 @@ struct msm_pingroup {
>   * @ngroups:	    The numbmer of entries in @groups.
>   * @ngpio:	    The number of pingroups the driver should expose as GPIOs.
>   * @pull_no_keeper: The SoC does not support keeper bias.
> + * @pdc_map:	    The map of GPIOs to the always-on PDC interrupt lines.
> + * @npdc_pins:	    The number of GPIOs mapped to the PDC pins in @pdc_map.
>   */
>  struct msm_pinctrl_soc_data {
>  	const struct pinctrl_pin_desc *pins;
> @@ -117,6 +129,8 @@ struct msm_pinctrl_soc_data {
>  	unsigned ngroups;
>  	unsigned ngpios;
>  	bool pull_no_keeper;
> +	struct msm_pinctrl_pdc_map *pdc_map;
> +	unsigned npdc_pins;
>  };
> 
>  int msm_pinctrl_probe(struct platform_device *pdev,

I find the whole thing terrifying, the most scary part being the
hand-crafted injection of the interrupt. I'd appreciate some insights
on how the pinctl HW is supposed to buffer things, and why its
summary IRQ isn't visible to the GIC after wakeup.

Thanks,

	M.

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

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

[Lina Iyer]

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.

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.

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

>> +
>> +	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);
>> +	unsigned pin, npins, irq;
>> +	struct wakeup_gpio_irq_map *p;
>> +	unsigned long flags, trigger;
>> +	const char *pin_name;
>> +	int i, ret;
>> +
>> +	pin = msm_gpio_get_pdc_pin(pctrl, d->hwirq);
>> +	if (pin < 0)
>> +		return 0;
>> +
>> +	npins = platform_irq_count(pdev);
>> +	if (npins <= 0)
>> +		return npins;
>> +
>> +	for (i = 0; i < npins; i++) {
>> +		irq = platform_get_irq(pdev, i);
>> +		if (irq >= 0 && pin == irq_get_irq_data(irq)->hwirq)
>> +			break;
>> +	}
>> +	if (i == npins)
>> +		return 0;
>> +
>> +	pin_name = kasprintf(GFP_KERNEL, "gpio-%lu", d->hwirq);
>> +	if (!pin_name)
>> +		return -ENOMEM;
>> +
>> +	trigger = irqd_get_trigger_type(d) | IRQF_ONESHOT | IRQF_NO_SUSPEND;
>> +	ret = request_irq(irq, wake_irq_gpio_handler, trigger, pin_name, d);
>> +	if (ret) {
>> +		pr_warn("GPIO-%lu could not be set up as wakeup", d->hwirq);
>> +		return ret;
>> +	}
>> +
>> +	p = kzalloc(sizeof(p), GFP_KERNEL);
>> +	if (!p)
>> +		return -ENOMEM;
>> +
>> +	p->pdc_irq = irq;
>> +	p->gpio = d->hwirq;
>> +	raw_spin_lock_irqsave(&pctrl->lock, flags);
>> +	list_add(&p->list, &pctrl->pdc_irqs);
>> +	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>
>This whole list business seems bizarre. Why don't you use the
>handler_data instead?
>
Ah, sure.

>> +
>> +	return 0;
>> +}
>> +
>> +static int msm_gpio_pdc_pin_release(struct irq_data *d)
>> +{
>> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>> +	struct wakeup_gpio_irq_map *p, *n, *t = NULL;
>> +	unsigned long flags;
>> +
>> +	raw_spin_lock_irqsave(&pctrl->lock, flags);
>> +	list_for_each_entry_safe(p, n, &pctrl->pdc_irqs, list) {
>> +		if (p->gpio == d->hwirq) {
>> +			list_del(&p->list);
>> +			t = p;
>> +			break;
>> +		}
>> +	}
>> +	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>> +	if (t) {
>> +		free_irq(t->pdc_irq, NULL);
>
>NULL? This should balance with the request_irq call, I believe.
>
Sorry, yes. I forgot to port the change from the test branch to here.

>> +		kfree(t);
>> +	}
>> +
>> +	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;
>> @@ -887,6 +1047,9 @@ 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;
>> +	INIT_LIST_HEAD(&pctrl->pdc_irqs);
>>
>>  	ret = gpiochip_add_data(&pctrl->chip, pctrl);
>>  	if (ret) {
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
>> index 9b9feea540ff..5b7f3160affe 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
>> @@ -97,6 +97,16 @@ struct msm_pingroup {
>>  	unsigned intr_detection_width:5;
>>  };
>>
>> +/**
>> + * struct msm_pinctrl_pdc_map - Map GPIOs to PDC pins on RPMH based SoCs
>> + * @hwirq:	   The GPIO that is mapped.
>> + * @pdc_pin:	   The PDC pin to with the GPIO IRQ line is routed.
>> + */
>> +struct msm_pinctrl_pdc_map {
>> +	u32 hwirq;
>> +	u32 pdc_pin;
>> +};
>> +
>>  /**
>>   * struct msm_pinctrl_soc_data - Qualcomm pin controller driver configuration
>>   * @pins:	    An array describing all pins the pin controller affects.
>> @@ -107,6 +117,8 @@ struct msm_pingroup {
>>   * @ngroups:	    The numbmer of entries in @groups.
>>   * @ngpio:	    The number of pingroups the driver should expose as GPIOs.
>>   * @pull_no_keeper: The SoC does not support keeper bias.
>> + * @pdc_map:	    The map of GPIOs to the always-on PDC interrupt lines.
>> + * @npdc_pins:	    The number of GPIOs mapped to the PDC pins in @pdc_map.
>>   */
>>  struct msm_pinctrl_soc_data {
>>  	const struct pinctrl_pin_desc *pins;
>> @@ -117,6 +129,8 @@ struct msm_pinctrl_soc_data {
>>  	unsigned ngroups;
>>  	unsigned ngpios;
>>  	bool pull_no_keeper;
>> +	struct msm_pinctrl_pdc_map *pdc_map;
>> +	unsigned npdc_pins;
>>  };
>>
>>  int msm_pinctrl_probe(struct platform_device *pdev,
>
>I find the whole thing terrifying, the most scary part being the
>hand-crafted injection of the interrupt. I'd appreciate some insights
>on how the pinctl HW is supposed to buffer things, and why its
>summary IRQ isn't visible to the GIC after wakeup.
>
Since the TLMM is not powered-on it will not sense the wakeup GPIO. The
summary IRQ is also not triggered. If there is a good way to get the
action->handler from the GPIO's latent IRQ and set it on the PDC IRQ, I
think it might work.

Thanks,
Lina

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

[Bjorn Andersson]

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

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

[Lina Iyer]

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

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

[Marc Zyngier]

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.

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

[Lina Iyer]

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

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

[Marc Zyngier]

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.

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

[Lina Iyer]

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.

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

[Stephen Boyd]

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?

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

[Marc Zyngier]

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.

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

[Stephen Boyd]

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.

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

[Marc Zyngier]

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.

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

[Stephen Boyd]

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?

Hooking system suspend in that case won't work. Is your hope that we can
avoid using hierarchical irqdomains here entirely?

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

[Lina Iyer]

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

[PATCH RESEND RFC 2/4] drivers: pinctrl: qcom: add wakeup gpio map for sdm845

[Lina Iyer]

Add GPIO to PDC pin map for the SDM845 SoC.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-sdm845.c | 76 +++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
index 2ab7a8885757..e93660922dc2 100644
--- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
+++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
@@ -1277,6 +1277,80 @@ static const struct msm_pingroup sdm845_groups[] = {
 	UFS_RESET(ufs_reset, 0x99f000),
 };

+static struct msm_pinctrl_pdc_map sdm845_wakeup_gpios[] = {
+	{1, 30},
+	{3, 31},
+	{5, 32},
+	{10, 33},
+	{11, 34},
+	{20, 35},
+	{22, 36},
+	{24, 37},
+	{26, 38},
+	{30, 39},
+	{32, 41},
+	{34, 42},
+	{36, 43},
+	{37, 44},
+	{38, 45},
+	{39, 46},
+	{40, 47},
+	{43, 49},
+	{44, 50},
+	{46, 51},
+	{48, 52},
+	{52, 54},
+	{53, 55},
+	{54, 56},
+	{56, 57},
+	{57, 58},
+	{58, 59},
+	{59, 60},
+	{60, 61},
+	{61, 62},
+	{62, 63},
+	{63, 64},
+	{64, 65},
+	{66, 66},
+	{68, 67},
+	{71, 68},
+	{73, 69},
+	{77, 70},
+	{78, 71},
+	{79, 72},
+	{80, 73},
+	{84, 74},
+	{85, 75},
+	{86, 76},
+	{88, 77},
+	{91, 79},
+	{92, 80},
+	{95, 81},
+	{96, 82},
+	{97, 83},
+	{101, 84},
+	{103, 85},
+	{104, 86},
+	{115, 90},
+	{116, 91},
+	{117, 92},
+	{118, 93},
+	{119, 94},
+	{120, 95},
+	{121, 96},
+	{122, 97},
+	{123, 98},
+	{124, 99},
+	{125, 100},
+	{127, 102},
+	{128, 103},
+	{129, 104},
+	{130, 105},
+	{132, 106},
+	{133, 107},
+	{145, 108},
+};
+
 static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
 	.pins = sdm845_pins,
 	.npins = ARRAY_SIZE(sdm845_pins),
@@ -1285,6 +1359,8 @@ static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
 	.groups = sdm845_groups,
 	.ngroups = ARRAY_SIZE(sdm845_groups),
 	.ngpios = 150,
+	.pdc_map = sdm845_wakeup_gpios,
+	.npdc_pins = ARRAY_SIZE(sdm845_wakeup_gpios),
 };

 static int sdm845_pinctrl_probe(struct platform_device *pdev)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH RESEND RFC 2/4] drivers: pinctrl: qcom: add wakeup gpio map for sdm845

[Marc Zyngier]

On Wed, 01 Aug 2018 03:00:19 +0100,
Lina Iyer <ilina@codeaurora.org> wrote:
> 
> Add GPIO to PDC pin map for the SDM845 SoC.
> 
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-sdm845.c | 76 +++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
> index 2ab7a8885757..e93660922dc2 100644
> --- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
> +++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
> @@ -1277,6 +1277,80 @@ static const struct msm_pingroup sdm845_groups[] = {
>  	UFS_RESET(ufs_reset, 0x99f000),
>  };
> 
> +static struct msm_pinctrl_pdc_map sdm845_wakeup_gpios[] = {

[huge array]

> +};

Why isn't that array part of the DT? I'd expect other SoCs to
eventually use a similar mechanism, no?

	M.

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

Re: [PATCH RESEND RFC 2/4] drivers: pinctrl: qcom: add wakeup gpio map for sdm845

[Lina Iyer]

On Wed, Aug 01 2018 at 02:42 -0600, Marc Zyngier wrote:
>On Wed, 01 Aug 2018 03:00:19 +0100,
>Lina Iyer <ilina@codeaurora.org> wrote:
>>
>> Add GPIO to PDC pin map for the SDM845 SoC.
>>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>>  drivers/pinctrl/qcom/pinctrl-sdm845.c | 76 +++++++++++++++++++++++++++
>>  1 file changed, 76 insertions(+)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
>> index 2ab7a8885757..e93660922dc2 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
>> +++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
>> @@ -1277,6 +1277,80 @@ static const struct msm_pingroup sdm845_groups[] = {
>>  	UFS_RESET(ufs_reset, 0x99f000),
>>  };
>>
>> +static struct msm_pinctrl_pdc_map sdm845_wakeup_gpios[] = {
>
>[huge array]
>
>> +};
>
>Why isn't that array part of the DT? I'd expect other SoCs to
>eventually use a similar mechanism, no?
>
I agree and it should be.

One place I am thinking is to add it to the DT definition of PDC
controller as a data argument -

	tlmm: pinctrl@000000{
	[...]
		interrupts-extended = <&pdc 30 IRQ_TYPE_LEVEL_HIGH 1>,
                      <&pdc 31 IRQ_TYPE_LEVEL_HIGH 3>,
		      <&pdc 32 IRQ_TYPE_LEVEL_HIGH 5>,
						   ^
						   |--- Provide the GPIO
						   for the PDC pin here.
	};

	pdc: interrupt-controller@b220000 {
		compatible = "qcom,sdm845-pdc";
		reg = <0xb220000 0x30000>;
		qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
		#interrupt-cells = <3>; <-------- Increase this from 2 ?
		interrupt-parent = <&intc>;
		interrupt-controller;
	};

Would that be acceptable?

Thanks,
Lina

Re: [PATCH RESEND RFC 2/4] drivers: pinctrl: qcom: add wakeup gpio map for sdm845

[Lina Iyer]

On Wed, Aug 01 2018 at 14:04 -0600, Lina Iyer wrote:
>On Wed, Aug 01 2018 at 02:42 -0600, Marc Zyngier wrote:
>>On Wed, 01 Aug 2018 03:00:19 +0100,
>>Lina Iyer <ilina@codeaurora.org> wrote:
>>>
>>>Add GPIO to PDC pin map for the SDM845 SoC.
>>>
>>>Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>>>---
>>> drivers/pinctrl/qcom/pinctrl-sdm845.c | 76 +++++++++++++++++++++++++++
>>> 1 file changed, 76 insertions(+)
>>>
>>>diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
>>>index 2ab7a8885757..e93660922dc2 100644
>>>--- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
>>>+++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
>>>@@ -1277,6 +1277,80 @@ static const struct msm_pingroup sdm845_groups[] = {
>>> 	UFS_RESET(ufs_reset, 0x99f000),
>>> };
>>>
>>>+static struct msm_pinctrl_pdc_map sdm845_wakeup_gpios[] = {
>>
>>[huge array]
>>
>>>+};
>>
>>Why isn't that array part of the DT? I'd expect other SoCs to
>>eventually use a similar mechanism, no?
>>
>I agree and it should be.
>
>One place I am thinking is to add it to the DT definition of PDC
>controller as a data argument -
>
>	tlmm: pinctrl@000000{
>	[...]
>		interrupts-extended = <&pdc 30 IRQ_TYPE_LEVEL_HIGH 1>,
>                     <&pdc 31 IRQ_TYPE_LEVEL_HIGH 3>,
>		      <&pdc 32 IRQ_TYPE_LEVEL_HIGH 5>,
>						   ^
>						   |--- Provide the GPIO
>						   for the PDC pin here.
>	};
>
>	pdc: interrupt-controller@b220000 {
>		compatible = "qcom,sdm845-pdc";
>		reg = <0xb220000 0x30000>;
>		qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
>		#interrupt-cells = <3>; <-------- Increase this from 2 ?
>		interrupt-parent = <&intc>;
>		interrupt-controller;
>	};
>
>Would that be acceptable?
>
Any ideas on how to do this better?
>Thanks,
>Lina

Re: [PATCH RESEND RFC 2/4] drivers: pinctrl: qcom: add wakeup gpio map for sdm845

[Marc Zyngier]

On 13/08/18 20:41, Lina Iyer wrote:
> On Wed, Aug 01 2018 at 14:04 -0600, Lina Iyer wrote:
>> On Wed, Aug 01 2018 at 02:42 -0600, Marc Zyngier wrote:
>>> On Wed, 01 Aug 2018 03:00:19 +0100,
>>> Lina Iyer <ilina@codeaurora.org> wrote:
>>>>
>>>> Add GPIO to PDC pin map for the SDM845 SoC.
>>>>
>>>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>>>> ---
>>>> drivers/pinctrl/qcom/pinctrl-sdm845.c | 76 +++++++++++++++++++++++++++
>>>> 1 file changed, 76 insertions(+)
>>>>
>>>> diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
>>>> index 2ab7a8885757..e93660922dc2 100644
>>>> --- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
>>>> +++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
>>>> @@ -1277,6 +1277,80 @@ static const struct msm_pingroup sdm845_groups[] = {
>>>> 	UFS_RESET(ufs_reset, 0x99f000),
>>>> };
>>>>
>>>> +static struct msm_pinctrl_pdc_map sdm845_wakeup_gpios[] = {
>>>
>>> [huge array]
>>>
>>>> +};
>>>
>>> Why isn't that array part of the DT? I'd expect other SoCs to
>>> eventually use a similar mechanism, no?
>>>
>> I agree and it should be.
>>
>> One place I am thinking is to add it to the DT definition of PDC
>> controller as a data argument -
>>
>> 	tlmm: pinctrl@000000{
>> 	[...]
>> 		interrupts-extended = <&pdc 30 IRQ_TYPE_LEVEL_HIGH 1>,
>>                     <&pdc 31 IRQ_TYPE_LEVEL_HIGH 3>,
>> 		      <&pdc 32 IRQ_TYPE_LEVEL_HIGH 5>,
>> 						   ^
>> 						   |--- Provide the GPIO
>> 						   for the PDC pin here.
>> 	};
>>
>> 	pdc: interrupt-controller@b220000 {
>> 		compatible = "qcom,sdm845-pdc";
>> 		reg = <0xb220000 0x30000>;
>> 		qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
>> 		#interrupt-cells = <3>; <-------- Increase this from 2 ?
>> 		interrupt-parent = <&intc>;
>> 		interrupt-controller;
>> 	};
>>
>> Would that be acceptable?
>>
> Any ideas on how to do this better?

I don't think adding an extra argument to the PDC interrupt specifier is
that great. I'd rather see some associated array in the PDC binding
mapping an interrupt to a pin on which special treatment must be applied.

Thanks,

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

[PATCH RESEND RFC 3/4] arm64: dts: msm: add PDC device bindings for sdm845

[Lina Iyer]

Add PDC interrupt controller device bindings for SDM845.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 2acc17ce1a9c..8ccce42885c1 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1213,6 +1213,15 @@
 			};
 		};

+		pdc: interrupt-controller@b220000 {
+			compatible = "qcom,sdm845-pdc";
+			reg = <0xb220000 0x30000>;
+			qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
+			#interrupt-cells = <2>;
+			interrupt-parent = <&intc>;
+			interrupt-controller;
+		};
+
 		timer@17c90000 {
 			#address-cells = <1>;
 			#size-cells = <1>;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH RESEND RFC 4/4] arm64: dts: qcom: add wake up interrupts for GPIOs

[Lina Iyer]

GPIOs that are wakeup capable have interrupt lines that are routed to
the always-on interrupt controller (PDC) in parallel to the pinctrl. The
interrupts listed here are the wake up lines corresponding to GPIOs.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 69 ++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 8ccce42885c1..96ef18ced85b 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -720,6 +720,75 @@
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
+			interrupts-extended = <&pdc 30 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 31 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 32 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 33 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 34 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 35 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 36 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 37 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 38 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 39 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 41 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 42 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 43 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 44 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 45 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 46 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 47 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 49 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 50 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 51 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 52 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 54 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 55 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 56 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 57 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 58 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 59 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 60 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 61 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 62 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 63 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 64 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 65 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 66 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 67 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 68 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 69 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 70 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 71 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 72 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 73 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 74 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 75 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 76 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 77 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 79 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 80 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 81 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 82 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 83 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 84 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 85 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 86 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 90 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 91 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 92 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 95 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 96 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 97 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 98 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 99 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 100 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 102 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 103 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 104 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 105 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 106 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 107 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 108 IRQ_TYPE_LEVEL_HIGH>;

 			qup_i2c0_default: qup-i2c0-default {
 				pinmux {
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project