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

Message ID 20181229114755.8711-3-masneyb@onstation.org
State New
Headers show
Series
  • qcom: spmi: add support for hierarchical IRQ chip
Related show

Commit Message

Brian Masney Dec. 29, 2018, 11:47 a.m.
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(-)

Comments

Stephen Boyd Jan. 4, 2019, 12:48 a.m. | #1
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,
Brian Masney Jan. 5, 2019, 12:08 p.m. | #2
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
Brian Masney Jan. 5, 2019, 12:51 p.m. | #3
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

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;
 }