From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Boyd Subject: Re: [PATCH 5/7] drivers: pinctrl: msm: setup GPIO irqchip hierarchy Date: Thu, 20 Dec 2018 12:03:33 -0800 Message-ID: <154533621302.79149.15228907259643696166@swboyd.mtv.corp.google.com> References: <20181219221105.3004-1-ilina@codeaurora.org> <20181219221105.3004-6-ilina@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <20181219221105.3004-6-ilina@codeaurora.org> Sender: linux-kernel-owner@vger.kernel.org To: evgreen@chromium.org, marc.zyngier@arm.com Cc: linux-kernel@vger.kernel.org, rplsssn@codeaurora.org, linux-arm-msm@vger.kernel.org, thierry.reding@gmail.com, bjorn.andersson@linaro.org, Lina Iyer List-Id: linux-arm-msm@vger.kernel.org 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. >=20 > 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. >=20 > Signed-off-by: Stephen Boyd 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 and I'm not sure how much I wrote vs. you wrote anymore so perhaps also add a Co-developed-by: Stephen Boyd And finally, please just email my chromium.org 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 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 changes. > @@ -967,11 +994,86 @@ static bool msm_gpio_needs_valid_mask(struct msm_pi= nctrl *pctrl) > return device_property_read_u16_array(pctrl->dev, "gpios", NULL, = 0) > 0; > } > =20 > +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 =3D fwspec->param[0]; > + *type =3D 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 =3D domain->host_data; > + struct msm_pinctrl *pctrl =3D gpiochip_get_data(gc); > + struct irq_fwspec *fwspec =3D arg; > + struct qcom_irq_fwspec parent =3D { }; > + unsigned int type; > + > + ret =3D msm_gpio_domain_translate(domain, fwspec, &hwirq, &type); > + if (ret) > + return ret; > + > + ret =3D 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 =3D domain->parent->fwnode; > + parent.fwspec.param_count =3D 2; > + parent.fwspec.param[0] =3D hwirq; > + parent.fwspec.param[1] =3D type; > + > + ret =3D irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &pare= nt); > + 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. > + */ > +static int msm_gpio_to_irq(struct gpio_chip *chip, unsigned int offset) > +{ > + struct irq_fwspec fwspec; > + > + fwspec.fwnode =3D of_node_to_fwnode(chip->of_node); > + fwspec.param[0] =3D offset; > + fwspec.param[1] =3D IRQ_TYPE_LEVEL_HIGH; > + fwspec.param_count =3D 2; > + > + return irq_create_fwspec_mapping(&fwspec); > +} > + > +static const struct irq_domain_ops msm_gpio_domain_ops =3D { > + .translate =3D msm_gpio_domain_translate, > + .alloc =3D msm_gpio_domain_alloc, > + .free =3D irq_domain_free_irqs_top, > +}; > +