Re: [PATCH 5/7] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

From: Lina Iyer
Date: Mon Jan 07 2019 - 13:54:45 EST

On Thu, Dec 20 2018 at 13:03 -0700, Stephen Boyd wrote:
Quoting Lina Iyer (2018-12-19 14:11:03)
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.

Signed-off-by: Stephen Boyd <sboyd@xxxxxxxxxx>

I don't think I gave a signed-off-by, so you need to ask to forge my
sign off here. Please change it to be:

Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>

and I'm not sure how much I wrote vs. you wrote anymore so perhaps also
add a

Co-developed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>

I meant to do that. I will add this tag instead. Sorry about that.

And finally, please just email my email for this series
because I apparently messed up the address once and now it's all going
to the wrong inbox. Thanks!


Signed-off-by: Lina Iyer <ilina@xxxxxxxxxxxxxx>

Can you Cc Linus Walleij and Bjorn Andersson on the whole patch series
next time? Would be good to have their review on major pinctrl driver

Bjorn is already included. Will make sure to include Linus in the next

@@ -967,11 +994,86 @@ 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;

Maybe this can be a generic function in gpiolib?

+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;
+ parent.fwspec.fwnode = domain->parent->fwnode;
+ parent.fwspec.param_count = 2;
+ parent.fwspec.param[0] = hwirq;
+ parent.fwspec.param[1] = type;
+ 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()

Hmm.. yeah we need to do this still. I think we can have a generic two
cell function similar to irq_domain_xlate_twocell() that does the fwspec
creation and uses some of the things that we pass to
gpiochip_irqchip_add(), like the default level type. This existing
function is not good to have, so there's work to do to get rid of this.

I was also thinking that maybe we can make the alloc function above take
a struct gpio_irq_fwspec structure that tells the alloc function what
gpiochip the irq is for. That would mean that we need to change the
gpio_to_irq() function below to be generic and stuff the chip inside the
fwspec wrapper structure:

struct gpio_irq_fwspec {
struct irq_fwspec fwspec;
struct gpio_chip *chip;
unsigned int offset;

but I seem to recall that was not working for some reason.

I didn't attemp this. I am hoping we can solve this even after this
patch set separately. That said, I will try this.

+ */
+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,