On Wed, Dec 26, 2018 at 06:30:20PM +0100, Linus Walleij wrote:
> > Do I need to in the parent IRQ domain somehow translate the parent virq
> > to the child virq and call generic_handle_irq() on that child virq? Or
> > something else?
>
> To me the method you're using looks perfectly acceptable (also
> doing all work inside the irqchip, as I wanted Thierry to do in his
> more centralized approach) but what is the "right approach" can
> only be answered by the irqchip maintainers. (Hi Marc!)
>
> If they have no time to look at it I will have a closer look
> when you post the final patch set and determine if I think
> it's the right approach... please keep Thierry looped in as
> well. I don't know if this will be merged first and then we
> patch it to use Thierry's hierarchical irqdomain handling in
> the gpiolib core, or if Thierry's patches go first so you can
> use his work.
>
> In any case I bet he is curious to see how you did it so I
> will forward the attachments to Thierry!
Sorry but I should have been a little clearer that this is not
completely working yet for IRQs setup as a hierarchy, which is why I
haven't send out an official v1 patch series yet. /proc/interrupts
shows 0 interrupts fired for volume up/down after I press the buttons.
The wrong interrupt is fired. It's firing the virq associated with
the parent domain (54 on spmi-gpio) instead of the virq associated
with the child domain (110 on spmi-arb), which is why gpio-keys does not
see the interrupts. I reattached my original message since the
debugging the I included there may be useful.
I'm unsure of how to map the virq in the parent to the child.
Brian
On Mon, Dec 17, 2018 at 02:00:14PM +0100, Linus Walleij wrote:
> Tell me if you need some specific help!
Hi Linus,
I attached my latest work. I have the IRQ hierarchy created, and
interrupts are now working properly on spmi-arb for the ADCs and other
devices. When I press the volume up button on the phone, I see an
interrupt fire on the parent IRQ domain (Linux virq 54 on spmi-arb)
instead of on the child IRQ domain (Linux virq 110 on spmi-gpio). Here
is some information from the board that shows that the IRQ hierarchy is
setup:
/ # grep volume /proc/interrupts
110: 0 0 spmi-gpio 1 Edge volume_up
111: 0 0 spmi-gpio 2 Edge volume_down
# Note: 54/55 are the parent virqs and they don't show up in
# /proc/interrupts as I would expect.
/ # 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
Do I need to in the parent IRQ domain somehow translate the parent virq
to the child virq and call generic_handle_irq() on that child virq? Or
something else?
Brian
>From 3d74d23bac7d4bbaa1612b4eeb11ed0e8d969bdd Mon Sep 17 00:00:00 2001
From: Brian Masney <masneyb@xxxxxxxxxxxxx>
Date: Tue, 25 Dec 2018 19:39:05 -0500
Subject: [PATCH 1/3] spmi: pmic-arb: convert to v2 irq interfaces to support
hierarchical IRQ chips
This patch is a work in progress that converts the IRQ code to use the
version 2 interfaces in order to support hierarchical IRQ chips. Code
was tested on a LG Nexus 5 (hammerhead) phone.
Signed-off-by: Brian Masney <masneyb@xxxxxxxxxxxxx>
---
drivers/spmi/spmi-pmic-arb.c | 70 +++++++++++++++++++++---------------
1 file changed, 42 insertions(+), 28 deletions(-)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 360b8218f322..bec7a94547c6 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_dbg(&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,35 @@ 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 int qpnpint_irq_domain_alloc(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs,
+ void *data)
{
- struct spmi_pmic_arb *pmic_arb = d->host_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;
+
+ if (WARN_ON(nr_irqs != 1))
+ return -EINVAL;
+
+ ret = qpnpint_irq_domain_translate(domain, fwspec, &hwirq, &type);
+ if (ret)
+ return ret;
- dev_dbg(&pmic_arb->spmic->dev, "virq = %u, hwirq = %lu\n", virq, hwirq);
+ irq_domain_set_info(domain, virq, hwirq, &pmic_arb_irqchip, pmic_arb,
+ handle_level_irq, NULL, NULL);
- irq_set_chip_and_handler(virq, &pmic_arb_irqchip, handle_level_irq);
- irq_set_chip_data(virq, d->host_data);
- irq_set_noprobe(virq);
return 0;
}
@@ -1126,8 +1138,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
>From 1bac1d35e084bf626dbb68facf752d0304d85d50 Mon Sep 17 00:00:00 2001
From: Brian Masney <masneyb@xxxxxxxxxxxxx>
Date: Tue, 25 Dec 2018 19:42:40 -0500
Subject: [PATCH 2/3] qcom: spmi-gpio: convert to hierarchical IRQ chip
This is a work in progress that adds support for hierarchical irqs.
Signed-off-by: Brian Masney <masneyb@xxxxxxxxxxxxx>
---
drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 120 ++++++++++++++++++++++-
1 file changed, 116 insertions(+), 4 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index 4458d44dfcf6..e45b415c8d1b 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,92 @@ 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;
+
+ if (WARN_ON(nr_irqs != 1))
+ return -EINVAL;
+
+ ret = pmic_gpio_domain_translate(domain, fwspec, &hwirq, &type);
+ if (ret)
+ return ret;
+
+ irq_domain_set_info(domain, virq, hwirq, &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 +1114,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 +1160,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 +1170,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
>From 718427e670d2210dc50ec02d2e02f890fc7453dd Mon Sep 17 00:00:00 2001
From: Brian Masney <masneyb@xxxxxxxxxxxxx>
Date: Tue, 25 Dec 2018 19:47:11 -0500
Subject: [PATCH 3/3] irq hierarchy test
This is what I am using to test that hierarchical irqs are working
correctly on spmi-gpio without calling gpio[d]_to_irq().
Since pmic_gpio_to_irq() now calls irq_create_fwspec_mapping() in this
patch series, /proc/interrupts correctly shows spmi-gpio as the owner of
the interrupt.
This particular patch won't be submitted for inclusion in upstream once
this series is ready.
Signed-off-by: Brian Masney <masneyb@xxxxxxxxxxxxx>
---
arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts | 6 ++++--
arch/arm/boot/dts/qcom-pm8941.dtsi | 3 +++
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
index 551d262a044a..92991a985864 100644
--- a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
+++ b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
@@ -333,14 +333,16 @@
volume-up {
label = "volume_up";
- gpios = <&pm8941_gpios 2 GPIO_ACTIVE_LOW>;
+ //gpios = <&pm8941_gpios 2 GPIO_ACTIVE_LOW>;
+ interrupts-extended = <&pm8941_gpios 0 1 0 IRQ_TYPE_EDGE_BOTH>;
linux,input-type = <1>;
linux,code = <KEY_VOLUMEUP>;
};
volume-down {
label = "volume_down";
- gpios = <&pm8941_gpios 3 GPIO_ACTIVE_LOW>;
+ //gpios = <&pm8941_gpios 3 GPIO_ACTIVE_LOW>;
+ interrupts-extended = <&pm8941_gpios 0 2 0 IRQ_TYPE_EDGE_BOTH>;
linux,input-type = <1>;
linux,code = <KEY_VOLUMEDOWN>;
};
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
