[PATCH 0/3] qcom: spmi: add support for hierarchical IRQ chip

[PATCH 0/3] qcom: spmi: add support for hierarchical IRQ chip

[Brian Masney]

This patch series adds hierarchical IRQ chip support to spmi-gpio so
that device tree consumers can request an IRQ directly from the GPIO
block rather than having to request an IRQ from the underlying PMIC.
The first patch in this series converts pmic-arb to use the version 2
IRQ interface and patches 2 and 3 add support for the hierarchical
IRQ chip to spmi-gpio.

For more background information, see the email thread with Linus
Walleij's excellent description of the problem at
https://www.spinics.net/lists/linux-gpio/msg34655.html.

The patches need to be applied in the order that I have them in order
to allow for a proper bisection, however part of the series can be
applied without any breakage. Maybe the spmi-arb change can go in
for v4.21 and the spmi-gpio changes can go in for v4.22?

This work was tested on a LG Nexus 5 (hammerhead) phone. My status page
at https://masneyb.github.io/nexus-5-upstream/ describes what is working
so far with an upstream kernel.

Brian Masney (3):
  spmi: pmic-arb: convert to v2 irq interfaces to support hierarchical
    IRQ chips
  qcom: spmi-gpio: add support for hierarchical IRQ chip
  ARM: dts: qcom: msm8974: add interrupt properties

 arch/arm/boot/dts/qcom-pm8941.dtsi       |   3 +
 drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 119 ++++++++++++++++++++++-
 drivers/spmi/spmi-pmic-arb.c             |  91 ++++++++++++-----
 3 files changed, 182 insertions(+), 31 deletions(-)

-- 
2.17.2

[PATCH 1/3] spmi: pmic-arb: convert to v2 irq interfaces to support hierarchical IRQ chips

[Brian Masney]

Convert the spmi-pmic-arb IRQ code to use the version 2 IRQ interface
in order to support hierarchical IRQ chips. Code was tested on a LG
Nexus 5 (hammerhead) phone.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/spmi/spmi-pmic-arb.c | 91 +++++++++++++++++++++++++-----------
 1 file changed, 64 insertions(+), 27 deletions(-)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 360b8218f322..c651d19f0623 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -666,7 +666,8 @@ static int qpnpint_get_irqchip_state(struct irq_data *d,
 	return 0;
 }
 
-static int qpnpint_irq_request_resources(struct irq_data *d)
+static int qpnpint_irq_domain_activate(struct irq_domain *domain,
+				       struct irq_data *d, bool reserve)
 {
 	struct spmi_pmic_arb *pmic_arb = irq_data_get_irq_chip_data(d);
 	u16 periph = hwirq_to_per(d->hwirq);
@@ -692,36 +693,37 @@ static struct irq_chip pmic_arb_irqchip = {
 	.irq_set_type	= qpnpint_irq_set_type,
 	.irq_set_wake	= qpnpint_irq_set_wake,
 	.irq_get_irqchip_state	= qpnpint_get_irqchip_state,
-	.irq_request_resources = qpnpint_irq_request_resources,
 	.flags		= IRQCHIP_MASK_ON_SUSPEND,
 };
 
-static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
-					   struct device_node *controller,
-					   const u32 *intspec,
-					   unsigned int intsize,
-					   unsigned long *out_hwirq,
-					   unsigned int *out_type)
+static int qpnpint_irq_domain_translate(struct irq_domain *d,
+					struct irq_fwspec *fwspec,
+					unsigned long *out_hwirq,
+					unsigned int *out_type)
 {
 	struct spmi_pmic_arb *pmic_arb = d->host_data;
 	u16 apid, ppid;
 	int rc;
 
-	dev_dbg(&pmic_arb->spmic->dev, "intspec[0] 0x%1x intspec[1] 0x%02x intspec[2] 0x%02x\n",
-		intspec[0], intspec[1], intspec[2]);
+	dev_dbg(&pmic_arb->spmic->dev,
+		"param[0] 0x%1x param[1] 0x%02x param[2] 0x%02x\n",
+		fwspec->param[0], fwspec->param[1], fwspec->param[2]);
 
-	if (irq_domain_get_of_node(d) != controller)
+	if (irq_domain_get_of_node(d) != pmic_arb->spmic->dev.of_node)
 		return -EINVAL;
-	if (intsize != 4)
+	if (fwspec->param_count != 4)
 		return -EINVAL;
-	if (intspec[0] > 0xF || intspec[1] > 0xFF || intspec[2] > 0x7)
+	if (fwspec->param[0] > 0xF || fwspec->param[1] > 0xFF ||
+	    fwspec->param[2] > 0x7)
 		return -EINVAL;
 
-	ppid = intspec[0] << 8 | intspec[1];
+	ppid = fwspec->param[0] << 8 | fwspec->param[1];
 	rc = pmic_arb->ver_ops->ppid_to_apid(pmic_arb, ppid);
 	if (rc < 0) {
-		dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph = %#x, irq = %u rc = %d\n",
-		intspec[0], intspec[1], intspec[2], rc);
+		dev_err(&pmic_arb->spmic->dev,
+			"failed to xlate sid = %#x, periph = %#x, irq = %u rc = %d\n",
+			fwspec->param[0], fwspec->param[1], fwspec->param[2],
+			rc);
 		return rc;
 	}
 
@@ -732,25 +734,58 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
 	if (apid < pmic_arb->min_apid)
 		pmic_arb->min_apid = apid;
 
-	*out_hwirq = spec_to_hwirq(intspec[0], intspec[1], intspec[2], apid);
-	*out_type  = intspec[3] & IRQ_TYPE_SENSE_MASK;
+	*out_hwirq = spec_to_hwirq(fwspec->param[0], fwspec->param[1],
+				   fwspec->param[2], apid);
+	*out_type  = fwspec->param[3] & IRQ_TYPE_SENSE_MASK;
 
 	dev_dbg(&pmic_arb->spmic->dev, "out_hwirq = %lu\n", *out_hwirq);
 
 	return 0;
 }
 
-static int qpnpint_irq_domain_map(struct irq_domain *d,
-				  unsigned int virq,
-				  irq_hw_number_t hwirq)
+
+static void qpnpint_irq_domain_map(struct spmi_pmic_arb *pmic_arb,
+				   struct irq_domain *domain, unsigned int virq,
+				   irq_hw_number_t hwirq)
 {
-	struct spmi_pmic_arb *pmic_arb = d->host_data;
+	unsigned int old_virq;
 
 	dev_dbg(&pmic_arb->spmic->dev, "virq = %u, hwirq = %lu\n", virq, hwirq);
 
-	irq_set_chip_and_handler(virq, &pmic_arb_irqchip, handle_level_irq);
-	irq_set_chip_data(virq, d->host_data);
-	irq_set_noprobe(virq);
+	/*
+	 * Check to see if the hwirq is already associated with another virq on
+	 * this IRQ domain. If so, then disassociate it before associating the
+	 * hwirq with the new virq. IRQs are all initially setup without an IRQ
+	 * hierarchy when this driver is probed and when mfd/qcom-spmi-pmic.c is
+	 * probed. Later in the boot process, an IRQ hierarchy is requested by
+	 * pinctrl-spmi-gpio.c, and the same hwirq is now associated with a new
+	 * virq.
+	 */
+	old_virq = irq_find_mapping(domain, hwirq);
+	if (old_virq)
+		irq_domain_disassociate(domain, old_virq);
+
+	irq_domain_set_info(domain, virq, hwirq, &pmic_arb_irqchip, pmic_arb,
+			    handle_level_irq, NULL, NULL);
+}
+
+static int qpnpint_irq_domain_alloc(struct irq_domain *domain,
+				    unsigned int virq, unsigned int nr_irqs,
+				    void *data)
+{
+	struct spmi_pmic_arb *pmic_arb = domain->host_data;
+	struct irq_fwspec *fwspec = data;
+	irq_hw_number_t hwirq;
+	unsigned int type;
+	int ret, i;
+
+	ret = qpnpint_irq_domain_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < nr_irqs; i++)
+		qpnpint_irq_domain_map(pmic_arb, domain, virq + i, hwirq + i);
+
 	return 0;
 }
 
@@ -1126,8 +1161,10 @@ static const struct pmic_arb_ver_ops pmic_arb_v5 = {
 };
 
 static const struct irq_domain_ops pmic_arb_irq_domain_ops = {
-	.map	= qpnpint_irq_domain_map,
-	.xlate	= qpnpint_irq_domain_dt_translate,
+	.activate = qpnpint_irq_domain_activate,
+	.alloc = qpnpint_irq_domain_alloc,
+	.free = irq_domain_free_irqs_common,
+	.translate = qpnpint_irq_domain_translate,
 };
 
 static int spmi_pmic_arb_probe(struct platform_device *pdev)
-- 
2.17.2

Re: [PATCH 1/3] spmi: pmic-arb: convert to v2 irq interfaces to support hierarchical IRQ chips

[Stephen Boyd]

Quoting Brian Masney (2018-12-29 03:47:53)
> Convert the spmi-pmic-arb IRQ code to use the version 2 IRQ interface
> in order to support hierarchical IRQ chips. Code was tested on a LG
> Nexus 5 (hammerhead) phone.

Can you add the motivation for this change here? Why should we care?

> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  drivers/spmi/spmi-pmic-arb.c | 91 +++++++++++++++++++++++++-----------
>  1 file changed, 64 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 360b8218f322..c651d19f0623 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -666,7 +666,8 @@ static int qpnpint_get_irqchip_state(struct irq_data *d,
>         return 0;
>  }
>  
> -static int qpnpint_irq_request_resources(struct irq_data *d)
> +static int qpnpint_irq_domain_activate(struct irq_domain *domain,
> +                                      struct irq_data *d, bool reserve)
>  {
>         struct spmi_pmic_arb *pmic_arb = irq_data_get_irq_chip_data(d);
>         u16 periph = hwirq_to_per(d->hwirq);
> @@ -692,36 +693,37 @@ static struct irq_chip pmic_arb_irqchip = {
>         .irq_set_type   = qpnpint_irq_set_type,
>         .irq_set_wake   = qpnpint_irq_set_wake,
>         .irq_get_irqchip_state  = qpnpint_get_irqchip_state,
> -       .irq_request_resources = qpnpint_irq_request_resources,
>         .flags          = IRQCHIP_MASK_ON_SUSPEND,
>  };
>  
> -static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
> -                                          struct device_node *controller,
> -                                          const u32 *intspec,
> -                                          unsigned int intsize,
> -                                          unsigned long *out_hwirq,
> -                                          unsigned int *out_type)
> +static int qpnpint_irq_domain_translate(struct irq_domain *d,
> +                                       struct irq_fwspec *fwspec,
> +                                       unsigned long *out_hwirq,
> +                                       unsigned int *out_type)
>  {
>         struct spmi_pmic_arb *pmic_arb = d->host_data;
>         u16 apid, ppid;
>         int rc;
>  
> -       dev_dbg(&pmic_arb->spmic->dev, "intspec[0] 0x%1x intspec[1] 0x%02x intspec[2] 0x%02x\n",
> -               intspec[0], intspec[1], intspec[2]);
> +       dev_dbg(&pmic_arb->spmic->dev,
> +               "param[0] 0x%1x param[1] 0x%02x param[2] 0x%02x\n",
> +               fwspec->param[0], fwspec->param[1], fwspec->param[2]);
>  
> -       if (irq_domain_get_of_node(d) != controller)
> +       if (irq_domain_get_of_node(d) != pmic_arb->spmic->dev.of_node)
>                 return -EINVAL;
> -       if (intsize != 4)
> +       if (fwspec->param_count != 4)
>                 return -EINVAL;
> -       if (intspec[0] > 0xF || intspec[1] > 0xFF || intspec[2] > 0x7)
> +       if (fwspec->param[0] > 0xF || fwspec->param[1] > 0xFF ||
> +           fwspec->param[2] > 0x7)
>                 return -EINVAL;
>  
> -       ppid = intspec[0] << 8 | intspec[1];
> +       ppid = fwspec->param[0] << 8 | fwspec->param[1];
>         rc = pmic_arb->ver_ops->ppid_to_apid(pmic_arb, ppid);
>         if (rc < 0) {
> -               dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph = %#x, irq = %u rc = %d\n",
> -               intspec[0], intspec[1], intspec[2], rc);
> +               dev_err(&pmic_arb->spmic->dev,
> +                       "failed to xlate sid = %#x, periph = %#x, irq = %u rc = %d\n",
> +                       fwspec->param[0], fwspec->param[1], fwspec->param[2],

Why not assign a u32 *intspec to fwspec->param and then not change many
lines in this function? Reducing the diff is useful to avoid glossing
over some mistake in variable renames.

> +                       rc);
>                 return rc;
>         }
>  
> @@ -732,25 +734,58 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
>         if (apid < pmic_arb->min_apid)
>                 pmic_arb->min_apid = apid;
>  
> -       *out_hwirq = spec_to_hwirq(intspec[0], intspec[1], intspec[2], apid);
> -       *out_type  = intspec[3] & IRQ_TYPE_SENSE_MASK;
> +       *out_hwirq = spec_to_hwirq(fwspec->param[0], fwspec->param[1],
> +                                  fwspec->param[2], apid);
> +       *out_type  = fwspec->param[3] & IRQ_TYPE_SENSE_MASK;
>  
>         dev_dbg(&pmic_arb->spmic->dev, "out_hwirq = %lu\n", *out_hwirq);
>  
>         return 0;
>  }
>  
> -static int qpnpint_irq_domain_map(struct irq_domain *d,
> -                                 unsigned int virq,
> -                                 irq_hw_number_t hwirq)
> +
> +static void qpnpint_irq_domain_map(struct spmi_pmic_arb *pmic_arb,
> +                                  struct irq_domain *domain, unsigned int virq,
> +                                  irq_hw_number_t hwirq)
>  {
> -       struct spmi_pmic_arb *pmic_arb = d->host_data;
> +       unsigned int old_virq;
>  
>         dev_dbg(&pmic_arb->spmic->dev, "virq = %u, hwirq = %lu\n", virq, hwirq);
>  
> -       irq_set_chip_and_handler(virq, &pmic_arb_irqchip, handle_level_irq);
> -       irq_set_chip_data(virq, d->host_data);
> -       irq_set_noprobe(virq);
> +       /*
> +        * Check to see if the hwirq is already associated with another virq on
> +        * this IRQ domain. If so, then disassociate it before associating the
> +        * hwirq with the new virq. IRQs are all initially setup without an IRQ
> +        * hierarchy when this driver is probed and when mfd/qcom-spmi-pmic.c is
> +        * probed. Later in the boot process, an IRQ hierarchy is requested by
> +        * pinctrl-spmi-gpio.c, and the same hwirq is now associated with a new
> +        * virq.

Is that because we count the irqs in the pmic gpio driver? It looks like
we should remove that code from the driver then and use the compatible
string to figure out how many gpios are present. It will make this
comment and disassociate code unnecessary, but unfortunately it will
mean we have to add all the compatible strings to the pmic gpio driver
just so we know how many gpios are there.

> +        */
> +       old_virq = irq_find_mapping(domain, hwirq);
> +       if (old_virq)
> +               irq_domain_disassociate(domain, old_virq);
> +
> +       irq_domain_set_info(domain, virq, hwirq, &pmic_arb_irqchip, pmic_arb,
> +                           handle_level_irq, NULL, NULL);
> +}
> +
> +static int qpnpint_irq_domain_alloc(struct irq_domain *domain,
> +                                   unsigned int virq, unsigned int nr_irqs,
> +                                   void *data)
> +{
> +       struct spmi_pmic_arb *pmic_arb = domain->host_data;
> +       struct irq_fwspec *fwspec = data;
> +       irq_hw_number_t hwirq;
> +       unsigned int type;
> +       int ret, i;
> +
> +       ret = qpnpint_irq_domain_translate(domain, fwspec, &hwirq, &type);
> +       if (ret)
> +               return ret;
> +
> +       for (i = 0; i < nr_irqs; i++)
> +               qpnpint_irq_domain_map(pmic_arb, domain, virq + i, hwirq + i);
> +
>         return 0;
>  }
>  

Otherwise patch looks good to me. Thanks for working on this. Are you
going to convert the ssbi master and gpio chip too?

Re: [PATCH 1/3] spmi: pmic-arb: convert to v2 irq interfaces to support hierarchical IRQ chips

[Brian Masney]

On Fri, Jan 04, 2019 at 04:25:23PM -0800, Stephen Boyd wrote:
> > +       /*
> > +        * Check to see if the hwirq is already associated with another virq on
> > +        * this IRQ domain. If so, then disassociate it before associating the
> > +        * hwirq with the new virq. IRQs are all initially setup without an IRQ
> > +        * hierarchy when this driver is probed and when mfd/qcom-spmi-pmic.c is
> > +        * probed. Later in the boot process, an IRQ hierarchy is requested by
> > +        * pinctrl-spmi-gpio.c, and the same hwirq is now associated with a new
> > +        * virq.
> 
> Is that because we count the irqs in the pmic gpio driver?

Yes.

The call to devm_of_platform_populate() in drivers/mfd/qcom-spmi-pmic.c
also triggers this condition. I'll experiment with this some more this
weekend.

> Otherwise patch looks good to me. Thanks for working on this. Are you
> going to convert the ssbi master and gpio chip too?

Yes, I will do that work as well once this patch series is accepted.
I currently don't have the hardware to test it, but I'm willing to pick
up a cheap device on ebay. Any suggestions for something that boots a
mainline kernel with the ssbi? It looks like the Sony Xperia Z phone is
one of the supported devices.

Thanks for your review on the other patches. It all makes sense.

Brian

Re: [PATCH 1/3] spmi: pmic-arb: convert to v2 irq interfaces to support hierarchical IRQ chips

[Linus Walleij]

On Sat, Jan 5, 2019 at 2:45 AM Brian Masney <masneyb@onstation.org> wrote:
> [Stephen]
> > Otherwise patch looks good to me. Thanks for working on this. Are you
> > going to convert the ssbi master and gpio chip too?
>
> Yes, I will do that work as well once this patch series is accepted.
> I currently don't have the hardware to test it, but I'm willing to pick
> up a cheap device on ebay. Any suggestions for something that boots a
> mainline kernel with the ssbi? It looks like the Sony Xperia Z phone is
> one of the supported devices.

I think my APQ8060 DragonBoard uses SSBI:
https://dflund.se/~triad/krad/dragonboard/
arch/arm/boot/dts/qcom-apq8060-dragonboard.dts

So I can test it. Or even  write the patch once the SSPI is merged and
known working.

Yours,
Linus Walleij

[PATCH 2/3] qcom: spmi-gpio: add support for hierarchical IRQ chip

[Brian Masney]

spmi-gpio did not have any irqchip support so consumers of this in
device tree would need to call gpio[d]_to_irq() in order to get the
proper IRQ on the underlying PMIC. IRQ chips in device tree should
be usable from the start without the consumer having to make an
additional call to get the proper IRQ on the parent. This patch adds
hierarchical IRQ chip support to the spmi-gpio code to correct this
issue.

Driver was tested using the volume buttons (via gpio-keys) on the LG
Nexus 5 (hammerhead) phone with the following two configurations.

volume-up {
	interrupts-extended = <&pm8941_gpios 0 1 0 IRQ_TYPE_EDGE_BOTH>;
	...
};

volume-up {
	gpios = <&pm8941_gpios 2 GPIO_ACTIVE_LOW>;
	...
};

Both configurations now show that spmi-gpio is the IRQ domain and that
the IRQ is setup in a hierarchy.

$ grep volume_up /proc/interrupts
110:          0          0  spmi-gpio   1 Edge      volume_up

$ cat /sys/kernel/debug/irq/irqs/110
handler:  handle_edge_irq
device:   (null)
status:   0x00000403
            _IRQ_NOPROBE
istate:   0x00000000
ddepth:   0
wdepth:   0
dstate:   0x02400203
            IRQ_TYPE_EDGE_RISING
            IRQ_TYPE_EDGE_FALLING
            IRQD_ACTIVATED
            IRQD_IRQ_STARTED
node:     0
affinity: 0-3
effectiv:
domain:  :soc:spmi@fc4cf000:pm8941@0:gpios@c000
 hwirq:   0x1
 chip:    spmi-gpio
  flags:   0x4
             IRQCHIP_MASK_ON_SUSPEND
 parent:
    domain:  :soc:spmi@fc4cf000
     hwirq:   0xc100057
     chip:    pmic_arb
      flags:   0x4
                 IRQCHIP_MASK_ON_SUSPEND

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 119 ++++++++++++++++++++++-
 1 file changed, 115 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index 4458d44dfcf6..0b46e24d0f8a 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -12,6 +12,7 @@
  */
 
 #include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
@@ -179,6 +180,8 @@ struct pmic_gpio_state {
 	struct regmap	*map;
 	struct pinctrl_dev *ctrl;
 	struct gpio_chip chip;
+	struct fwnode_handle *fwnode;
+	struct irq_domain *domain;
 };
 
 static const struct pinconf_generic_params pmic_gpio_bindings[] = {
@@ -761,11 +764,16 @@ static int pmic_gpio_of_xlate(struct gpio_chip *chip,
 static int pmic_gpio_to_irq(struct gpio_chip *chip, unsigned pin)
 {
 	struct pmic_gpio_state *state = gpiochip_get_data(chip);
-	struct pmic_gpio_pad *pad;
+	struct irq_fwspec fwspec;
 
-	pad = state->ctrl->desc->pins[pin].drv_data;
+	fwspec.fwnode = state->fwnode;
+	fwspec.param_count = 4;
+	fwspec.param[0] = 0;
+	fwspec.param[1] = pin;
+	fwspec.param[2] = 0;
+	fwspec.param[3] = IRQ_TYPE_NONE;
 
-	return pad->irq;
+	return irq_create_fwspec_mapping(&fwspec);
 }
 
 static void pmic_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
@@ -935,8 +943,91 @@ static int pmic_gpio_populate(struct pmic_gpio_state *state,
 	return 0;
 }
 
+static struct irq_chip pmic_gpio_irq_chip = {
+	.name = "spmi-gpio",
+	.irq_ack = irq_chip_ack_parent,
+	.irq_mask = irq_chip_mask_parent,
+	.irq_unmask = irq_chip_unmask_parent,
+	.irq_set_type = irq_chip_set_type_parent,
+	.irq_set_wake = irq_chip_set_wake_parent,
+	.flags = IRQCHIP_MASK_ON_SUSPEND,
+};
+
+static int pmic_gpio_irq_activate(struct irq_domain *domain,
+				  struct irq_data *data, bool reserve)
+{
+	struct pmic_gpio_state *state = domain->host_data;
+
+	return gpiochip_lock_as_irq(&state->chip, data->hwirq);
+}
+
+static void pmic_gpio_irq_deactivate(struct irq_domain *domain,
+				     struct irq_data *data)
+{
+	struct pmic_gpio_state *state = domain->host_data;
+
+	gpiochip_unlock_as_irq(&state->chip, data->hwirq);
+}
+
+static int pmic_gpio_domain_translate(struct irq_domain *domain,
+				      struct irq_fwspec *fwspec,
+				      unsigned long *hwirq,
+				      unsigned int *type)
+{
+	struct pmic_gpio_state *state = domain->host_data;
+
+	if ((fwspec->param_count != 4) ||
+	    (fwspec->param[1] >= state->chip.ngpio))
+		return -EINVAL;
+
+	*hwirq = fwspec->param[1];
+	*type = fwspec->param[3];
+
+	return 0;
+}
+
+static int pmic_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				  unsigned int nr_irqs, void *data)
+{
+	struct pmic_gpio_state *state = domain->host_data;
+	struct irq_fwspec *fwspec = data;
+	struct irq_fwspec parent_fwspec;
+	irq_hw_number_t hwirq;
+	unsigned int type;
+	int ret, i;
+
+	ret = pmic_gpio_domain_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_set_info(domain, virq + i, hwirq + i,
+				    &pmic_gpio_irq_chip, state,
+				    handle_level_irq, NULL, NULL);
+
+	parent_fwspec.fwnode = domain->parent->fwnode;
+	parent_fwspec.param_count = 4;
+	parent_fwspec.param[0] = fwspec->param[0];
+	parent_fwspec.param[1] = fwspec->param[1] + 0xc0;
+	parent_fwspec.param[2] = fwspec->param[2];
+	parent_fwspec.param[3] = fwspec->param[3];
+
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
+					    &parent_fwspec);
+}
+
+static const struct irq_domain_ops pmic_gpio_domain_ops = {
+	.activate = pmic_gpio_irq_activate,
+	.alloc = pmic_gpio_domain_alloc,
+	.deactivate = pmic_gpio_irq_deactivate,
+	.free = irq_domain_free_irqs_common,
+	.translate = pmic_gpio_domain_translate,
+};
+
 static int pmic_gpio_probe(struct platform_device *pdev)
 {
+	struct irq_domain *parent_domain;
+	struct device_node *parent_node;
 	struct device *dev = &pdev->dev;
 	struct pinctrl_pin_desc *pindesc;
 	struct pinctrl_desc *pctrldesc;
@@ -1022,10 +1113,27 @@ static int pmic_gpio_probe(struct platform_device *pdev)
 	if (IS_ERR(state->ctrl))
 		return PTR_ERR(state->ctrl);
 
+	parent_node = of_irq_find_parent(state->dev->of_node);
+	if (!parent_node)
+		return -ENXIO;
+
+	parent_domain = irq_find_host(parent_node);
+	if (!parent_domain)
+		return -ENXIO;
+
+	state->fwnode = of_node_to_fwnode(state->dev->of_node);
+	state->domain = irq_domain_create_hierarchy(parent_domain, 0,
+						    state->chip.ngpio,
+						    state->fwnode,
+						    &pmic_gpio_domain_ops,
+						    state);
+	if (!state->domain)
+		return -ENODEV;
+
 	ret = gpiochip_add_data(&state->chip, state);
 	if (ret) {
 		dev_err(state->dev, "can't add gpio chip\n");
-		return ret;
+		goto err_chip_add_data;
 	}
 
 	/*
@@ -1051,6 +1159,8 @@ static int pmic_gpio_probe(struct platform_device *pdev)
 
 err_range:
 	gpiochip_remove(&state->chip);
+err_chip_add_data:
+	irq_domain_remove(state->domain);
 	return ret;
 }
 
@@ -1059,6 +1169,7 @@ static int pmic_gpio_remove(struct platform_device *pdev)
 	struct pmic_gpio_state *state = platform_get_drvdata(pdev);
 
 	gpiochip_remove(&state->chip);
+	irq_domain_remove(state->domain);
 	return 0;
 }
 
-- 
2.17.2

Re: [PATCH 2/3] qcom: spmi-gpio: add support for hierarchical IRQ chip

[Stephen Boyd]

Quoting Brian Masney (2018-12-29 03:47:54)
> spmi-gpio did not have any irqchip support so consumers of this in
> device tree would need to call gpio[d]_to_irq() in order to get the
> proper IRQ on the underlying PMIC. IRQ chips in device tree should
> be usable from the start without the consumer having to make an
> additional call to get the proper IRQ on the parent. This patch adds
> hierarchical IRQ chip support to the spmi-gpio code to correct this
> issue.
> 
> Driver was tested using the volume buttons (via gpio-keys) on the LG
> Nexus 5 (hammerhead) phone with the following two configurations.
> 
> volume-up {
>         interrupts-extended = <&pm8941_gpios 0 1 0 IRQ_TYPE_EDGE_BOTH>;

I'd think we want the interrupt-cells for the pmic gpio controller to be
2 cells (pin and flags) instead of 4 like you have here to match the
parent interrupt specifier. The gpio chip never spans two SIDs so that
cell isn't important to express to each GPIO consumer, and the other
zero cell will always be zero for the gpio because it's a 1 to 1
relation between the gpio and SPMI interrupt number. Basically there
isn't more than one irq per gpio in the PMIC so it will always be zero
for that cell too. I'd expect to see something like:

	interrupts-extended = <&pm8941_gpios 1 IRQ_TYPE_EDGE_BOTH>

to indicate that GPIO1 should be requested. I also seem to recall that
GPIO numbering starts from 1 instead of 0, so please keep that in mind.

>         ...
> };
> 
> volume-up {
>         gpios = <&pm8941_gpios 2 GPIO_ACTIVE_LOW>;
>         ...
> };
> 
> Both configurations now show that spmi-gpio is the IRQ domain and that
> the IRQ is setup in a hierarchy.
> 
> $ grep volume_up /proc/interrupts
> 110:          0          0  spmi-gpio   1 Edge      volume_up
> 
> $ cat /sys/kernel/debug/irq/irqs/110
> handler:  handle_edge_irq
> device:   (null)
> status:   0x00000403
>             _IRQ_NOPROBE
> istate:   0x00000000
> ddepth:   0
> wdepth:   0
> dstate:   0x02400203
>             IRQ_TYPE_EDGE_RISING
>             IRQ_TYPE_EDGE_FALLING
>             IRQD_ACTIVATED
>             IRQD_IRQ_STARTED
> node:     0
> affinity: 0-3
> effectiv:
> domain:  :soc:spmi@fc4cf000:pm8941@0:gpios@c000
>  hwirq:   0x1
>  chip:    spmi-gpio
>   flags:   0x4
>              IRQCHIP_MASK_ON_SUSPEND
>  parent:
>     domain:  :soc:spmi@fc4cf000
>      hwirq:   0xc100057
>      chip:    pmic_arb
>       flags:   0x4
>                  IRQCHIP_MASK_ON_SUSPEND
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>


> @@ -761,11 +764,16 @@ static int pmic_gpio_of_xlate(struct gpio_chip *chip,
>  static int pmic_gpio_to_irq(struct gpio_chip *chip, unsigned pin)
>  {
>         struct pmic_gpio_state *state = gpiochip_get_data(chip);
> -       struct pmic_gpio_pad *pad;
> +       struct irq_fwspec fwspec;
>  
> -       pad = state->ctrl->desc->pins[pin].drv_data;
> +       fwspec.fwnode = state->fwnode;
> +       fwspec.param_count = 4;
> +       fwspec.param[0] = 0;
> +       fwspec.param[1] = pin;
> +       fwspec.param[2] = 0;
> +       fwspec.param[3] = IRQ_TYPE_NONE;
>  
> -       return pad->irq;
> +       return irq_create_fwspec_mapping(&fwspec);
>  }
>  
>  static void pmic_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> @@ -935,8 +943,91 @@ static int pmic_gpio_populate(struct pmic_gpio_state *state,
>         return 0;
>  }
>  
> +static struct irq_chip pmic_gpio_irq_chip = {
> +       .name = "spmi-gpio",
> +       .irq_ack = irq_chip_ack_parent,
> +       .irq_mask = irq_chip_mask_parent,
> +       .irq_unmask = irq_chip_unmask_parent,
> +       .irq_set_type = irq_chip_set_type_parent,
> +       .irq_set_wake = irq_chip_set_wake_parent,
> +       .flags = IRQCHIP_MASK_ON_SUSPEND,
> +};
> +
> +static int pmic_gpio_irq_activate(struct irq_domain *domain,
> +                                 struct irq_data *data, bool reserve)
> +{
> +       struct pmic_gpio_state *state = domain->host_data;

How about just storing the gpiochip in the domain->host_data?

> +
> +       return gpiochip_lock_as_irq(&state->chip, data->hwirq);
> +}
> +
> +static void pmic_gpio_irq_deactivate(struct irq_domain *domain,
> +                                    struct irq_data *data)
> +{
> +       struct pmic_gpio_state *state = domain->host_data;
> +
> +       gpiochip_unlock_as_irq(&state->chip, data->hwirq);
> +}
> +

Then these could be generic gpiolib APIs?

> +static int pmic_gpio_domain_translate(struct irq_domain *domain,
> +                                     struct irq_fwspec *fwspec,
> +                                     unsigned long *hwirq,
> +                                     unsigned int *type)
> +{
> +       struct pmic_gpio_state *state = domain->host_data;
> +
> +       if ((fwspec->param_count != 4) ||
> +           (fwspec->param[1] >= state->chip.ngpio))

Please drop parenthesis here.

> +               return -EINVAL;
> +
> +       *hwirq = fwspec->param[1];
> +       *type = fwspec->param[3];
> +
> +       return 0;
> +}
> +
> +static int pmic_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +                                 unsigned int nr_irqs, void *data)
> +{
> +       struct pmic_gpio_state *state = domain->host_data;
> +       struct irq_fwspec *fwspec = data;
> +       struct irq_fwspec parent_fwspec;
> +       irq_hw_number_t hwirq;
> +       unsigned int type;
> +       int ret, i;
> +
> +       ret = pmic_gpio_domain_translate(domain, fwspec, &hwirq, &type);
> +       if (ret)
> +               return ret;
> +
> +       for (i = 0; i < nr_irqs; i++)
> +               irq_domain_set_info(domain, virq + i, hwirq + i,
> +                                   &pmic_gpio_irq_chip, state,
> +                                   handle_level_irq, NULL, NULL);

Does almost nobody pass a name for that last parameter?

> +
> +       parent_fwspec.fwnode = domain->parent->fwnode;
> +       parent_fwspec.param_count = 4;
> +       parent_fwspec.param[0] = fwspec->param[0];
> +       parent_fwspec.param[1] = fwspec->param[1] + 0xc0;

Ideally this comes from the reg property but it's always been 0xc0 so
OK.

> +       parent_fwspec.param[2] = fwspec->param[2];
> +       parent_fwspec.param[3] = fwspec->param[3];
> +
> +       return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
> +                                           &parent_fwspec);
> +}
> +
> +static const struct irq_domain_ops pmic_gpio_domain_ops = {
> +       .activate = pmic_gpio_irq_activate,
> +       .alloc = pmic_gpio_domain_alloc,
> +       .deactivate = pmic_gpio_irq_deactivate,
> +       .free = irq_domain_free_irqs_common,
> +       .translate = pmic_gpio_domain_translate,
> +};
> +
>  static int pmic_gpio_probe(struct platform_device *pdev)
>  {
> +       struct irq_domain *parent_domain;
> +       struct device_node *parent_node;
>         struct device *dev = &pdev->dev;
>         struct pinctrl_pin_desc *pindesc;
>         struct pinctrl_desc *pctrldesc;
> @@ -1022,10 +1113,27 @@ static int pmic_gpio_probe(struct platform_device *pdev)
>         if (IS_ERR(state->ctrl))
>                 return PTR_ERR(state->ctrl);
>  
> +       parent_node = of_irq_find_parent(state->dev->of_node);

Do we need to of_node_put() this pointer when we're done with it?

> +       if (!parent_node)
> +               return -ENXIO;
> +
> +       parent_domain = irq_find_host(parent_node);
> +       if (!parent_domain)
> +               return -ENXIO;
> +
> +       state->fwnode = of_node_to_fwnode(state->dev->of_node);
> +       state->domain = irq_domain_create_hierarchy(parent_domain, 0,
> +                                                   state->chip.ngpio,
> +                                                   state->fwnode,
> +                                                   &pmic_gpio_domain_ops,

Re: [PATCH 2/3] qcom: spmi-gpio: add support for hierarchical IRQ chip

[Brian Masney]

On Thu, Jan 03, 2019 at 04:48:33PM -0800, Stephen Boyd wrote:
> I'd think we want the interrupt-cells for the pmic gpio controller to be
> 2 cells (pin and flags) instead of 4 like you have here to match the
> parent interrupt specifier.

I originally went with 4 interrupt cells for spmi-gpio to match the
number of cells on the parent (spmi-arb). From qcom-msm8974.dtsi:

spmi_bus: spmi@fc4cf000 {
	compatible = "qcom,spmi-pmic-arb";
	interrupt-controller;
	#interrupt-cells = <4>;
	...
};

I agree that we should go with 2 cells for spmi-gpio.

> I also seem to recall that GPIO numbering starts from 1 instead of
> 0, so please keep that in mind.

I'm using the pinctrl numbering, which is zero based.

/ # head /sys/kernel/debug/pinctrl/fc4cf000.spmi\:pm8941@0\:gpios@c000/pins 
registered pins: 36
pin 0 (gpio1) 
pin 1 (gpio2) 
pin 2 (gpio3) 
pin 3 (gpio4) 
pin 4 (gpio5) 
pin 5 (gpio6) 
pin 6 (gpio7) 
pin 7 (gpio8) 
pin 8 (gpio9) 

> > +static int pmic_gpio_irq_activate(struct irq_domain *domain,
> > +                                 struct irq_data *data, bool reserve)
> > +{
> > +       struct pmic_gpio_state *state = domain->host_data;
> 
> How about just storing the gpiochip in the domain->host_data?
> 
> > +
> > +       return gpiochip_lock_as_irq(&state->chip, data->hwirq);
> > +}
> > +
> > +static void pmic_gpio_irq_deactivate(struct irq_domain *domain,
> > +                                    struct irq_data *data)
> > +{
> > +       struct pmic_gpio_state *state = domain->host_data;
> > +
> > +       gpiochip_unlock_as_irq(&state->chip, data->hwirq);
> > +}
> > +
> 
> Then these could be generic gpiolib APIs?

I tried this:

static const struct irq_domain_ops pmic_gpio_domain_ops = {
        .activate = gpiochip_lock_as_irq,
        .alloc = pmic_gpio_domain_alloc,
        .deactivate = gpiochip_unlock_as_irq,
        .free = irq_domain_free_irqs_common,
        .translate = pmic_gpio_domain_translate,
};

But get an incompatible pointer types compiler error.

drivers/pinctrl/qcom/pinctrl-spmi-gpio.c:1003:14: error: initialization of
‘int (*)(struct irq_domain *, struct irq_data *, bool)’ {aka ‘int
(*)(struct irq_domain *, struct irq_data *, _Bool)’} from incompatible
pointer type ‘int (*)(struct gpio_chip *, unsigned int)’
[-Werror=incompatible-pointer-types]


> > +static int pmic_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
> > +                                 unsigned int nr_irqs, void *data)
> > +{
> > +       struct pmic_gpio_state *state = domain->host_data;
> > +       struct irq_fwspec *fwspec = data;
> > +       struct irq_fwspec parent_fwspec;
> > +       irq_hw_number_t hwirq;
> > +       unsigned int type;
> > +       int ret, i;
> > +
> > +       ret = pmic_gpio_domain_translate(domain, fwspec, &hwirq, &type);
> > +       if (ret)
> > +               return ret;
> > +
> > +       for (i = 0; i < nr_irqs; i++)
> > +               irq_domain_set_info(domain, virq + i, hwirq + i,
> > +                                   &pmic_gpio_irq_chip, state,
> > +                                   handle_level_irq, NULL, NULL);
> 
> Does almost nobody pass a name for that last parameter?

I see 26 callers to irq_domain_set_info() outside this patch set and
only 3 of them actually set a name. I'm open to suggestions for what to
put here.

Brian

Re: [PATCH 2/3] qcom: spmi-gpio: add support for hierarchical IRQ chip

[Brian Masney]

On Sat, Jan 05, 2019 at 07:08:44AM -0500, Brian Masney wrote:
> > I also seem to recall that GPIO numbering starts from 1 instead of
> > 0, so please keep that in mind.
> 
> I'm using the pinctrl numbering, which is zero based.
> 
> / # head /sys/kernel/debug/pinctrl/fc4cf000.spmi\:pm8941@0\:gpios@c000/pins 
> registered pins: 36
> pin 0 (gpio1) 
> pin 1 (gpio2) 
> pin 2 (gpio3) 
> pin 3 (gpio4) 
> pin 4 (gpio5) 
> pin 5 (gpio6) 
> pin 6 (gpio7) 
> pin 7 (gpio8) 
> pin 8 (gpio9) 

After more thought: the pin numbering from pinctrl is an implementation
detail that device tree should not be aware of. This needs to be the
GPIO pin number. I'll correct this in v2.

Brian

[PATCH 3/3] ARM: dts: qcom: msm8974: add interrupt properties

[Brian Masney]

Add interrupt properties now that spmi-gpio is a proper hierarchical IRQ
chip.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 arch/arm/boot/dts/qcom-pm8941.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-pm8941.dtsi b/arch/arm/boot/dts/qcom-pm8941.dtsi
index 9a91b758f7aa..d0b7f0eff288 100644
--- a/arch/arm/boot/dts/qcom-pm8941.dtsi
+++ b/arch/arm/boot/dts/qcom-pm8941.dtsi
@@ -65,6 +65,9 @@
 			gpio-controller;
 			gpio-ranges = <&pm8941_gpios 0 0 36>;
 			#gpio-cells = <2>;
+			interrupt-parent = <&spmi_bus>;
+			interrupt-controller;
+			#interrupt-cells = <4>;
 			interrupts = <0 0xc0 0 IRQ_TYPE_NONE>,
 				     <0 0xc1 0 IRQ_TYPE_NONE>,
 				     <0 0xc2 0 IRQ_TYPE_NONE>,
-- 
2.17.2

Re: [PATCH 3/3] ARM: dts: qcom: msm8974: add interrupt properties

[Stephen Boyd]

Quoting Brian Masney (2018-12-29 03:47:55)
> Add interrupt properties now that spmi-gpio is a proper hierarchical IRQ
> chip.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  arch/arm/boot/dts/qcom-pm8941.dtsi | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/qcom-pm8941.dtsi b/arch/arm/boot/dts/qcom-pm8941.dtsi
> index 9a91b758f7aa..d0b7f0eff288 100644
> --- a/arch/arm/boot/dts/qcom-pm8941.dtsi
> +++ b/arch/arm/boot/dts/qcom-pm8941.dtsi
> @@ -65,6 +65,9 @@
>                         gpio-controller;
>                         gpio-ranges = <&pm8941_gpios 0 0 36>;
>                         #gpio-cells = <2>;
> +                       interrupt-parent = <&spmi_bus>;
> +                       interrupt-controller;
> +                       #interrupt-cells = <4>;
>                         interrupts = <0 0xc0 0 IRQ_TYPE_NONE>,
>                                      <0 0xc1 0 IRQ_TYPE_NONE>,
>                                      <0 0xc2 0 IRQ_TYPE_NONE>,

Shouldn't we remove the interrupts property here then? Although we use
it to count the number of gpios that are supported so I guess we'll have
to specify it all the time now just to know how many pins there are.
Alternatively we change the driver to know about all the different
compatibles and use the right number of gpios for each compatible string
that we see.

Re: [PATCH 0/3] qcom: spmi: add support for hierarchical IRQ chip

[Linus Walleij]

On Sat, Dec 29, 2018 at 12:48 PM Brian Masney <masneyb@onstation.org> wrote:

> This patch series adds hierarchical IRQ chip support to spmi-gpio so
> that device tree consumers can request an IRQ directly from the GPIO
> block rather than having to request an IRQ from the underlying PMIC.
> The first patch in this series converts pmic-arb to use the version 2
> IRQ interface and patches 2 and 3 add support for the hierarchical
> IRQ chip to spmi-gpio.

Very nice stuff!

> The patches need to be applied in the order that I have them in order
> to allow for a proper bisection, however part of the series can be
> applied without any breakage. Maybe the spmi-arb change can go in
> for v4.21 and the spmi-gpio changes can go in for v4.22?

I'm fairly sure the SPMI maintainer (Stephen Boyd) can just ACK the
patch once he's happy with it and I can then apply it all to
the pin control tree.

> This work was tested on a LG Nexus 5 (hammerhead) phone. My status page
> at https://masneyb.github.io/nexus-5-upstream/ describes what is working
> so far with an upstream kernel.

I'm a big fan of your work. I have a few of these devices as well,
so the day I manage to create a serial cable for it, I'll gladly test
this stuff and see what I can add.

Yours,
Linus Walleij