From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Zyngier Subject: Re: [PATCH v3 6/9] drivers: pinctrl: msm: setup GPIO irqchip hierarchy Date: Mon, 11 Mar 2019 10:54:27 +0000 Message-ID: <6452d538-5714-7e3a-1537-2dd1c4976653@arm.com> References: <20190222221850.26939-1-ilina@codeaurora.org> <20190222221850.26939-7-ilina@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20190222221850.26939-7-ilina@codeaurora.org> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Lina Iyer , swboyd@chromium.org, evgreen@chromium.org Cc: linux-kernel@vger.kernel.org, rplsssn@codeaurora.org, linux-arm-msm@vger.kernel.org, thierry.reding@gmail.com, bjorn.andersson@linaro.org, dianders@chromium.org, linus.walleij@linaro.org List-Id: linux-arm-msm@vger.kernel.org On 22/02/2019 22:18, Lina Iyer wrote: > 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. > > Co-developed-by: Stephen Boyd > Signed-off-by: Lina Iyer > --- > Changes in v4: > - Use of_irq_domain_map() and pass PDC pin to parent irqdomain > Changes in v3: > - Call parent mask when masking GPIO interrupt > Changes in v2: > - Fix bug when unmasking PDC interrupt > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 141 ++++++++++++++++++++++++++--- > 1 file changed, 128 insertions(+), 13 deletions(-) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index ee8119879c4c..83053b45982e 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -27,6 +28,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -69,6 +71,7 @@ struct msm_pinctrl { > > DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO); > DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO); > + DECLARE_BITMAP(wakeup_masked_irqs, MAX_NR_GPIO); > > const struct msm_pinctrl_soc_data *soc; > void __iomem *regs[MAX_NR_TILES]; > @@ -703,6 +706,13 @@ static void msm_gpio_irq_mask(struct irq_data *d) > > g = &pctrl->soc->groups[d->hwirq]; > > + if (d->parent_data) > + irq_chip_mask_parent(d); > + > + /* Monitored by parent wakeup controller?*/ > + if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs)) > + return; > + > raw_spin_lock_irqsave(&pctrl->lock, flags); > > val = msm_readl_intr_cfg(pctrl, g); > @@ -747,6 +757,13 @@ static void msm_gpio_irq_unmask(struct irq_data *d) > > g = &pctrl->soc->groups[d->hwirq]; > > + if (d->parent_data) > + irq_chip_unmask_parent(d); > + > + /* Monitored by parent wakeup controller? Keep masked */ > + if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs)) > + return; > + > raw_spin_lock_irqsave(&pctrl->lock, flags); > > val = msm_readl_intr_cfg(pctrl, g); > @@ -767,6 +784,10 @@ static void msm_gpio_irq_ack(struct irq_data *d) > unsigned long flags; > u32 val; > > + /* Handled by parent wakeup controller? Do nothing */ > + if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs)) > + return; > + > g = &pctrl->soc->groups[d->hwirq]; > > raw_spin_lock_irqsave(&pctrl->lock, flags); > @@ -794,6 +815,13 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) > > g = &pctrl->soc->groups[d->hwirq]; > > + if (d->parent_data) > + irq_chip_set_type_parent(d, type); > + > + /* Monitored by parent wakeup controller? Keep masked */ > + if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs)) > + return 0; > + > raw_spin_lock_irqsave(&pctrl->lock, flags); > > /* > @@ -890,6 +918,9 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) > > raw_spin_unlock_irqrestore(&pctrl->lock, flags); > > + if (d->parent_data) > + irq_chip_set_wake_parent(d, on); > + > return 0; > } > > @@ -967,11 +998,87 @@ 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; > +} > + > +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; > + > + ret = of_irq_domain_map(fwspec, &parent.fwspec); > + if (ret) > + return ret; > + > + parent.fwspec.fwnode = domain->parent->fwnode; > + > + 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() > + */ > +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, > +}; > + > static int msm_gpio_init(struct msm_pinctrl *pctrl) > { > struct gpio_chip *chip; > int ret; > unsigned ngpio = pctrl->soc->ngpios; > + struct device_node *dn; > > if (WARN_ON(ngpio > MAX_NR_GPIO)) > return -EINVAL; > @@ -986,6 +1093,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) > chip->need_valid_mask = msm_gpio_needs_valid_mask(pctrl); > > pctrl->irq_chip.name = "msmgpio"; > + pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent; > pctrl->irq_chip.irq_mask = msm_gpio_irq_mask; > pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask; > pctrl->irq_chip.irq_ack = msm_gpio_irq_ack; > @@ -994,6 +1102,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) > pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres; > pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres; > > + chip->irq.chip = &pctrl->irq_chip; > + chip->irq.domain_ops = &msm_gpio_domain_ops; > + chip->irq.handler = handle_edge_irq; > + chip->irq.default_type = IRQ_TYPE_NONE; I know you're only moving this around, but can you explain why you're setting IRQ_TYPE_NONE in combination with handle_edge_irq? The two really should go together. If this doesn't work for some reason, please document it. > + > + dn = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0); > + if (dn) { > + chip->irq.parent_domain = irq_find_matching_host(dn, > + DOMAIN_BUS_WAKEUP); > + of_node_put(dn); > + if (!chip->irq.parent_domain) > + return -EPROBE_DEFER; > + > + chip->to_irq = msm_gpio_to_irq; > + } > + > ret = gpiochip_add_data(&pctrl->chip, pctrl); > if (ret) { > dev_err(pctrl->dev, "Failed register gpiochip\n"); > @@ -1015,26 +1139,17 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) > dev_name(pctrl->dev), 0, 0, chip->ngpio); > if (ret) { > dev_err(pctrl->dev, "Failed to add pin range\n"); > - gpiochip_remove(&pctrl->chip); > - return ret; > + goto fail; > } > } > > - ret = gpiochip_irqchip_add(chip, > - &pctrl->irq_chip, > - 0, > - handle_edge_irq, > - IRQ_TYPE_NONE); > - if (ret) { > - dev_err(pctrl->dev, "Failed to add irqchip to gpiochip\n"); > - gpiochip_remove(&pctrl->chip); > - return -ENOSYS; > - } > - > gpiochip_set_chained_irqchip(chip, &pctrl->irq_chip, pctrl->irq, > msm_gpio_irq_handler); > > return 0; > +fail: > + gpiochip_remove(&pctrl->chip); > + return ret; > } > > static int msm_ps_hold_restart(struct notifier_block *nb, unsigned long action, > Thanks, M. -- Jazz is not dead. It just smells funny...