Patchwork [v4,07/10] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

login
register
mail settings
Submitter Lina Iyer
Date March 13, 2019, 9:18 p.m.
Message ID <20190313211844.29416-8-ilina@codeaurora.org>
Download mbox | patch
Permalink /patch/748435/
State New
Headers show

Comments

Lina Iyer - March 13, 2019, 9:18 p.m.
To allow GPIOs to wakeup the system from suspend or deep idle, the
wakeup capable GPIOs are setup in hierarchy with interrupts from the
wakeup-parent irqchip.

In older SoC's, the TLMM will handover detection to the parent irqchip
and in newer SoC's, the parent irqchip may also be active as well as the
TLMM and therefore the GPIOs need to be masked at TLMM to avoid
duplicate interrupts. To enable both these configurations to exist,
allow the parent irqchip to dictate the TLMM irqchip's behavior when
masking/unmasking the interrupt.

Co-developed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v4:
	- Remove irq_set_wake() on summary IRQ interrupt
Changes in v3:
	- Use of_irq_domain_map() and pass PDC pin to parent irqdomain
Changes in v2:
	- Call parent mask when masking GPIO interrupt
Changes in v1:
	- Fix bug when unmasking PDC interrupt
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 151 ++++++++++++++++++++++++-----
 1 file changed, 128 insertions(+), 23 deletions(-)
Stephen Boyd - March 15, 2019, 4:28 p.m.
Quoting Lina Iyer (2019-03-13 14:18:41)
> ---
> Changes in v4:
>         - Remove irq_set_wake() on summary IRQ interrupt
> Changes in v3:
>         - Use of_irq_domain_map() and pass PDC pin to parent irqdomain
> Changes in v2:
>         - Call parent mask when masking GPIO interrupt
> Changes in v1:
>         - Fix bug when unmasking PDC interrupt
[...]
> +}
> +
> +/*
> + * TODO: Get rid of this and push it into gpiochip_to_irq()
> + */

Any chance this TODO can be resolved?

> @@ -994,6 +1092,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>         pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
>         pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
>  
> +       chip->irq.chip = &pctrl->irq_chip;
> +       chip->irq.domain_ops = &msm_gpio_domain_ops;
> +       chip->irq.handler = handle_edge_irq;
> +       chip->irq.default_type = IRQ_TYPE_EDGE_RISING;

This also changed from v3. It used to be IRQ_TYPE_NONE. Specifying this
here seems to cause gpiolib to print a WARN.


        /*
         * Specifying a default trigger is a terrible idea if DT or ACPI is
         * used to configure the interrupts, as you may end up with
         * conflicting triggers. Tell the user, and reset to NONE.
         */
        if (WARN(np && type != IRQ_TYPE_NONE,
                 "%s: Ignoring %u default trigger\n", np->full_name, type))
                type = IRQ_TYPE_NONE;


So I guess this change should be dropped. Or at the least, it should be
split out to it's own patch and the motivations can be discussed in the
commit text.
Marc Zyngier - March 16, 2019, 11:39 a.m.
On Fri, 15 Mar 2019 09:28:31 -0700
Stephen Boyd <swboyd@chromium.org> wrote:

> Quoting Lina Iyer (2019-03-13 14:18:41)
> > ---
> > Changes in v4:
> >         - Remove irq_set_wake() on summary IRQ interrupt
> > Changes in v3:
> >         - Use of_irq_domain_map() and pass PDC pin to parent irqdomain
> > Changes in v2:
> >         - Call parent mask when masking GPIO interrupt
> > Changes in v1:
> >         - Fix bug when unmasking PDC interrupt  
> [...]
> > +}
> > +
> > +/*
> > + * TODO: Get rid of this and push it into gpiochip_to_irq()
> > + */  
> 
> Any chance this TODO can be resolved?
> 
> > @@ -994,6 +1092,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
> >         pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
> >         pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
> >  
> > +       chip->irq.chip = &pctrl->irq_chip;
> > +       chip->irq.domain_ops = &msm_gpio_domain_ops;
> > +       chip->irq.handler = handle_edge_irq;
> > +       chip->irq.default_type = IRQ_TYPE_EDGE_RISING;  
> 
> This also changed from v3. It used to be IRQ_TYPE_NONE. Specifying this
> here seems to cause gpiolib to print a WARN.
> 
> 
>         /*
>          * Specifying a default trigger is a terrible idea if DT or ACPI is
>          * used to configure the interrupts, as you may end up with
>          * conflicting triggers. Tell the user, and reset to NONE.
>          */
>         if (WARN(np && type != IRQ_TYPE_NONE,
>                  "%s: Ignoring %u default trigger\n", np->full_name, type))
>                 type = IRQ_TYPE_NONE;
> 
> 
> So I guess this change should be dropped. Or at the least, it should be
> split out to it's own patch and the motivations can be discussed in the
> commit text.

It is something I requested (although I expected this to be a
different patch, and even a clarification would have been OK).

One way or another, the default trigger must match the flow handler. If
we set it up with IRQ_TYPE_NONE, what does it mean? The fact that
IRQ_TYPE_NONE acts as a wildcard doesn't mean the handle_edge_irq flow
handler is a good match for all interrupt types (it is rarely OK for
level interrupts).

Thanks,

	M.
Lina Iyer - April 17, 2019, 4:08 p.m.
On Fri, Mar 15 2019 at 10:28 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2019-03-13 14:18:41)
>> ---
>> Changes in v4:
>>         - Remove irq_set_wake() on summary IRQ interrupt
>> Changes in v3:
>>         - Use of_irq_domain_map() and pass PDC pin to parent irqdomain
>> Changes in v2:
>>         - Call parent mask when masking GPIO interrupt
>> Changes in v1:
>>         - Fix bug when unmasking PDC interrupt
>[...]
>> +}
>> +
>> +/*
>> + * TODO: Get rid of this and push it into gpiochip_to_irq()
>> + */
>
>Any chance this TODO can be resolved?
>

I am thinking of something like this. Would there be any issue in
setting the type to IRQ_TYPE_SENSE_MASK instead of any one particular
type?

---8<-----
static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
{
#ifdef CONFIG_OF_GPIO
        struct irq_fwspec fwspec;

        if (chip->of_node) {
               fwspec.fwnode = of_node_to_fwnode(chip->of_node);
               fwspec.param[0] = offset;
               fwspec.param[1] = IRQ_TYPE_SENSE_MASK;
               fwspec.param_count = 2;
               return irq_create_fwspec_mapping(&fwspec);
        }
#endif

        if (!gpiochip_irqchip_irq_valid(chip, offset))
                return -ENXIO;

        return irq_create_mapping(chip->irq.domain, offset);
}
---8<----


Thanks,
Lina
Linus Walleij - April 17, 2019, 5:38 p.m.
On Wed, Apr 17, 2019 at 6:08 PM Lina Iyer <ilina@codeaurora.org> wrote:

> I am thinking of something like this. Would there be any issue in
> setting the type to IRQ_TYPE_SENSE_MASK instead of any one particular
> type?
>
> ---8<-----
> static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
> {
> #ifdef CONFIG_OF_GPIO
>         struct irq_fwspec fwspec;
>
>         if (chip->of_node) {
>                fwspec.fwnode = of_node_to_fwnode(chip->of_node);
>                fwspec.param[0] = offset;
>                fwspec.param[1] = IRQ_TYPE_SENSE_MASK;
>                fwspec.param_count = 2;
>                return irq_create_fwspec_mapping(&fwspec);
>         }
> #endif
>
>         if (!gpiochip_irqchip_irq_valid(chip, offset))
>                 return -ENXIO;
>
>         return irq_create_mapping(chip->irq.domain, offset);

Isn't the real problem that irq_create_mapping() isn't already doing this
for you?

chip->irq.domain is created with irq_domain_add_simple() and after
Thierrys patch with irq_domain_add_hierarchy(), so I think it is simply
that the hierarchical irqdomain helper code needs to go deeper here.

gpiolib needs struct irq_domain_ops for the hierarchical irqdomain
which implements generic versions of .translate(), .alloc() and .free().
I am working on this... albeit slowly :/

Yours,
Linus Walleij

Patch

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index ee8119879c4c..97e9000fa3c3 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -17,6 +17,7 @@ 
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/pinctrl/machine.h>
 #include <linux/pinctrl/pinctrl.h>
@@ -27,6 +28,7 @@ 
 #include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
 #include <linux/spinlock.h>
+#include <linux/soc/qcom/irq.h>
 #include <linux/reboot.h>
 #include <linux/pm.h>
 #include <linux/log2.h>
@@ -69,6 +71,7 @@  struct msm_pinctrl {
 
 	DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
 	DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
+	DECLARE_BITMAP(wakeup_masked_irqs, MAX_NR_GPIO);
 
 	const struct msm_pinctrl_soc_data *soc;
 	void __iomem *regs[MAX_NR_TILES];
@@ -703,6 +706,13 @@  static void msm_gpio_irq_mask(struct irq_data *d)
 
 	g = &pctrl->soc->groups[d->hwirq];
 
+	if (d->parent_data)
+		irq_chip_mask_parent(d);
+
+	/* Monitored by parent wakeup controller?*/
+	if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))
+		return;
+
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	val = msm_readl_intr_cfg(pctrl, g);
@@ -747,6 +757,13 @@  static void msm_gpio_irq_unmask(struct irq_data *d)
 
 	g = &pctrl->soc->groups[d->hwirq];
 
+	if (d->parent_data)
+		irq_chip_unmask_parent(d);
+
+	/* Monitored by parent wakeup controller? Keep masked */
+	if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))
+		return;
+
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	val = msm_readl_intr_cfg(pctrl, g);
@@ -767,6 +784,10 @@  static void msm_gpio_irq_ack(struct irq_data *d)
 	unsigned long flags;
 	u32 val;
 
+	/* Handled by parent wakeup controller? Do nothing */
+	if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))
+		return;
+
 	g = &pctrl->soc->groups[d->hwirq];
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
@@ -794,6 +815,13 @@  static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 
 	g = &pctrl->soc->groups[d->hwirq];
 
+	if (d->parent_data)
+		irq_chip_set_type_parent(d, type);
+
+	/* Monitored by parent wakeup controller? Keep masked */
+	if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))
+		return 0;
+
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	/*
@@ -880,17 +908,10 @@  static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 
 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;
+	if (d->parent_data)
+		return irq_chip_set_wake_parent(d, on);
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
-
-	irq_set_irq_wake(pctrl->irq, on);
-
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
-
-	return 0;
+	return -ENODEV;
 }
 
 static int msm_gpio_irq_reqres(struct irq_data *d)
@@ -967,11 +988,87 @@  static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
 	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
 }
 
+static int msm_gpio_domain_translate(struct irq_domain *d,
+				     struct irq_fwspec *fwspec,
+				     unsigned long *hwirq, unsigned int *type)
+{
+	if (is_of_node(fwspec->fwnode)) {
+		if (fwspec->param_count < 2)
+			return -EINVAL;
+		*hwirq = fwspec->param[0];
+		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				 unsigned int nr_irqs, void *arg)
+{
+	int ret;
+	irq_hw_number_t hwirq;
+	struct gpio_chip *gc = domain->host_data;
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	struct irq_fwspec *fwspec = arg;
+	struct qcom_irq_fwspec parent = { };
+	unsigned int type;
+
+	ret = msm_gpio_domain_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+					    &pctrl->irq_chip, gc);
+	if (ret < 0)
+		return ret;
+
+	if (!domain->parent)
+		return 0;
+
+	ret = of_irq_domain_map(fwspec, &parent.fwspec);
+	if (ret)
+		return ret;
+
+	parent.fwspec.fwnode = domain->parent->fwnode;
+
+	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent);
+	if (ret)
+		return ret;
+
+	if (parent.mask)
+		set_bit(hwirq, pctrl->wakeup_masked_irqs);
+
+	return 0;
+}
+
+/*
+ * TODO: Get rid of this and push it into gpiochip_to_irq()
+ */
+static int msm_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct irq_fwspec fwspec;
+
+	fwspec.fwnode = of_node_to_fwnode(chip->of_node);
+	fwspec.param[0] = offset;
+	fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH;
+	fwspec.param_count = 2;
+
+	return irq_create_fwspec_mapping(&fwspec);
+}
+
+static const struct irq_domain_ops msm_gpio_domain_ops = {
+	.translate = msm_gpio_domain_translate,
+	.alloc     = msm_gpio_domain_alloc,
+	.free      = irq_domain_free_irqs_top,
+};
+
 static int msm_gpio_init(struct msm_pinctrl *pctrl)
 {
 	struct gpio_chip *chip;
 	int ret;
 	unsigned ngpio = pctrl->soc->ngpios;
+	struct device_node *dn;
 
 	if (WARN_ON(ngpio > MAX_NR_GPIO))
 		return -EINVAL;
@@ -986,6 +1083,7 @@  static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	chip->need_valid_mask = msm_gpio_needs_valid_mask(pctrl);
 
 	pctrl->irq_chip.name = "msmgpio";
+	pctrl->irq_chip.irq_eoi	= irq_chip_eoi_parent;
 	pctrl->irq_chip.irq_mask = msm_gpio_irq_mask;
 	pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask;
 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
@@ -994,6 +1092,22 @@  static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
 	pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 
+	chip->irq.chip = &pctrl->irq_chip;
+	chip->irq.domain_ops = &msm_gpio_domain_ops;
+	chip->irq.handler = handle_edge_irq;
+	chip->irq.default_type = IRQ_TYPE_EDGE_RISING;
+
+	dn = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0);
+	if (dn) {
+		chip->irq.parent_domain = irq_find_matching_host(dn,
+						 DOMAIN_BUS_WAKEUP);
+		of_node_put(dn);
+		if (!chip->irq.parent_domain)
+			return -EPROBE_DEFER;
+
+		chip->to_irq = msm_gpio_to_irq;
+	}
+
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
 		dev_err(pctrl->dev, "Failed register gpiochip\n");
@@ -1015,26 +1129,17 @@  static int msm_gpio_init(struct msm_pinctrl *pctrl)
 			dev_name(pctrl->dev), 0, 0, chip->ngpio);
 		if (ret) {
 			dev_err(pctrl->dev, "Failed to add pin range\n");
-			gpiochip_remove(&pctrl->chip);
-			return ret;
+			goto fail;
 		}
 	}
 
-	ret = gpiochip_irqchip_add(chip,
-				   &pctrl->irq_chip,
-				   0,
-				   handle_edge_irq,
-				   IRQ_TYPE_NONE);
-	if (ret) {
-		dev_err(pctrl->dev, "Failed to add irqchip to gpiochip\n");
-		gpiochip_remove(&pctrl->chip);
-		return -ENOSYS;
-	}
-
 	gpiochip_set_chained_irqchip(chip, &pctrl->irq_chip, pctrl->irq,
 				     msm_gpio_irq_handler);
 
 	return 0;
+fail:
+	gpiochip_remove(&pctrl->chip);
+	return ret;
 }
 
 static int msm_ps_hold_restart(struct notifier_block *nb, unsigned long action,