| Message ID | 1573756521-27373-9-git-send-email-ilina@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | Support wakeup capable GPIOs | expand |
On Thu, Nov 14, 2019 at 7:35 PM Lina Iyer <ilina@codeaurora.org> wrote: > Some GPIOs are marked as wakeup capable and are routed to another > interrupt controller that is an always-domain and can detect interrupts > even most of the SoC is powered off. The wakeup interrupt controller > wakes up the GIC and replays the interrupt at the GIC. > > Setup the TLMM irqchip in hierarchy with the wakeup interrupt controller > and ensure the wakeup GPIOs are handled correctly. > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > Signed-off-by: Lina Iyer <ilina@codeaurora.org> This looks finished, and elegant. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Thu, Nov 14 2019 at 11:47 -0700, Linus Walleij wrote: >On Thu, Nov 14, 2019 at 7:35 PM Lina Iyer <ilina@codeaurora.org> wrote: > >> Some GPIOs are marked as wakeup capable and are routed to another >> interrupt controller that is an always-domain and can detect interrupts >> even most of the SoC is powered off. The wakeup interrupt controller >> wakes up the GIC and replays the interrupt at the GIC. >> >> Setup the TLMM irqchip in hierarchy with the wakeup interrupt controller >> and ensure the wakeup GPIOs are handled correctly. >> >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> > >This looks finished, and elegant. >Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Thanks Linus. -- Lina
Quoting Lina Iyer (2019-11-14 10:35:17) > Some GPIOs are marked as wakeup capable and are routed to another > interrupt controller that is an always-domain and can detect interrupts > even most of the SoC is powered off. The wakeup interrupt controller even when? > wakes up the GIC and replays the interrupt at the GIC. > > Setup the TLMM irqchip in hierarchy with the wakeup interrupt controller > and ensure the wakeup GPIOs are handled correctly. > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> Is it Co-developed-by for Maulik? > Signed-off-by: Lina Iyer <ilina@codeaurora.org> Some minor comments. Shouldn't be hard to fix and resend quickly I hope. > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index 763da0b..c245686 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -44,6 +46,7 @@ > * @enabled_irqs: Bitmap of currently enabled irqs. > * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge > * detection. > + * @skip_wake_irqs: Skip IRQs that are handled by wakeup interrupt contrroller s/contrroller/controller/ > * @soc; Reference to soc_data of platform specific data. > * @regs: Base addresses for the TLMM tiles. > */ > @@ -778,10 +794,37 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear) > > static void msm_gpio_irq_enable(struct irq_data *d) > { > + /* > + * Clear the interrupt that may be pending before we enable > + * the line. > + * This is especially a problem with the GPIOs routed to the > + * PDC. These GPIOs are direct-connect interrupts to the GIC. > + * Disabling the interrupt line at the PDC does not prevent > + * the interrupt from being latched at the GIC. The state at > + * GIC needs to be cleared before enabling. > + */ > + if (d->parent_data) { > + irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0); > + irq_chip_enable_parent(d); > + } > > msm_gpio_irq_clear_unmask(d, true); > } > > +static void msm_gpio_irq_disable(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct msm_pinctrl *pctrl = gpiochip_get_data(gc); > + > + if (d->parent_data) > + irq_chip_disable_parent(d); > + > + if (test_bit(d->hwirq, pctrl->skip_wake_irqs)) > + return; > + > + msm_gpio_irq_mask(d); Why not if (!test_bit(...) msm_gpio_irq_mask(d); > +} > + > static void msm_gpio_irq_unmask(struct irq_data *d) > { > msm_gpio_irq_clear_unmask(d, false); > @@ -912,6 +964,15 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) > struct msm_pinctrl *pctrl = gpiochip_get_data(gc); > unsigned long flags; > > + if (d->parent_data) > + irq_chip_set_wake_parent(d, on); > + > + /* > + * While they may not wake up when the TLMM is powered off, > + * some GPIOs would like to wakeup the system from suspend > + * when TLMM is powered on. To allow that, enable the GPIO > + * summary line to be wakeup capable at GIC. > + */ Can this comment go above the irq_set_irq_wake() line below instead of this spinlock? > raw_spin_lock_irqsave(&pctrl->lock, flags); > > irq_set_irq_wake(pctrl->irq, on); > @@ -990,6 +1051,30 @@ static void msm_gpio_irq_handler(struct irq_desc *desc) > chained_irq_exit(chip, desc); > } > > +static int msm_gpio_wakeirq(struct gpio_chip *gc, > + unsigned int child, > + unsigned int child_type, > + unsigned int *parent, > + unsigned int *parent_type) > +{ > + struct msm_pinctrl *pctrl = gpiochip_get_data(gc); > + const struct msm_gpio_wakeirq_map *map; > + int i; > + > + *parent = GPIO_NO_WAKE_IRQ; > + *parent_type = IRQ_TYPE_EDGE_RISING; > + > + for (i = 0; i < pctrl->soc->nwakeirq_map; i++) { > + map = &pctrl->soc->wakeirq_map[i]; > + if (map->gpio == child) { > + *parent = map->wakeirq; > + break; > + } > + } > + > + return 0; Shouldn't we return -EINVAL if we can't translate the gpio irq to the parent domain? I would expect to see return -EINVAL here and the above if condition to return 0 instead of break. > +} > + > static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl) > { > if (pctrl->soc->reserved_gpios) > @@ -1004,6 +1089,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) > struct gpio_irq_chip *girq; > int ret; > unsigned ngpio = pctrl->soc->ngpios; > + struct device_node *np; > > if (WARN_ON(ngpio > MAX_NR_GPIO)) > return -EINVAL; > @@ -1020,17 +1106,44 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) > > pctrl->irq_chip.name = "msmgpio"; > pctrl->irq_chip.irq_enable = msm_gpio_irq_enable; > + pctrl->irq_chip.irq_disable = msm_gpio_irq_disable; > 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; > + pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent; > pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type; > pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake; > pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres; > pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres; > > + np = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0); > + if (np) { > + int i; > + bool skip; > + unsigned int gpio; Can these be placed at the top of this function instead of buried halfway down here? > + > + chip->irq.parent_domain = irq_find_matching_host(np, > + DOMAIN_BUS_WAKEUP); > + of_node_put(np); > + if (!chip->irq.parent_domain) > + return -EPROBE_DEFER; > + chip->irq.child_to_parent_hwirq = msm_gpio_wakeirq; > + > + /* > + * Let's skip handling the GPIOs, if the parent irqchip > + * handling the direct connect IRQ of the GPIO. is handling? > + */ > + skip = irq_domain_qcom_handle_wakeup(chip->irq.parent_domain); > + for (i = 0; skip && i < pctrl->soc->nwakeirq_map; i++) { > + gpio = pctrl->soc->wakeirq_map[i].gpio; > + set_bit(gpio, pctrl->skip_wake_irqs); > + } > + } > + > girq = &chip->irq; > girq->chip = &pctrl->irq_chip; > girq->parent_handler = msm_gpio_irq_handler; > + girq->fwnode = pctrl->dev->fwnode; > girq->num_parents = 1; > girq->parents = devm_kcalloc(pctrl->dev, 1, sizeof(*girq->parents), > GFP_KERNEL); > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h > index 48569cda8..1547020 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.h > +++ b/drivers/pinctrl/qcom/pinctrl-msm.h > @@ -5,6 +5,8 @@ > #ifndef __PINCTRL_MSM_H__ > #define __PINCTRL_MSM_H__ > > +#include <linux/gpio/driver.h> What is this include for? > + > struct pinctrl_pin_desc; > > /** > @@ -101,6 +113,8 @@ struct msm_pingroup { > * @ngroups: The numbmer of entries in @groups. > * @ngpio: The number of pingroups the driver should expose as GPIOs. > * @pull_no_keeper: The SoC does not support keeper bias. > + * @wakeirq_map: The map of wakeup capable GPIOs and the pin at PDC/MPM > + * @nwakeirq_map: The number of entries in @hierarchy_map Is it 'number of entries in @wakeirq_map"? > */ > struct msm_pinctrl_soc_data { > const struct pinctrl_pin_desc *pins; > @@ -114,6 +128,8 @@ struct msm_pinctrl_soc_data { > const char *const *tiles; > unsigned int ntiles; > const int *reserved_gpios; > + const struct msm_gpio_wakeirq_map *wakeirq_map; > + unsigned int nwakeirq_map; > }; > > extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops; > diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h > index 637c0bf..e01391c 100644 > --- a/include/linux/soc/qcom/irq.h > +++ b/include/linux/soc/qcom/irq.h > @@ -18,4 +18,17 @@ > #define IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP (IRQ_DOMAIN_FLAG_NONCORE << 0) > #define IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP (IRQ_DOMAIN_FLAG_NONCORE << 1) > > +/** > + * irq_domain_qcom_handle_wakeup: Return if the domain handles interrupt > + * configuration > + * @d: irq domain > + * > + * This QCOM specific irq domain call returns if the interrupt controller > + * requires the interrupt be masked at the child interrupt controller. > + */ > +static inline bool irq_domain_qcom_handle_wakeup(struct irq_domain *d) could be const irq_domain here. > +{ > + return (d->flags & IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP); > +} > +
>Quoting Lina Iyer (2019-11-14 10:35:17) >> Some GPIOs are marked as wakeup capable and are routed to another >> interrupt controller that is an always-domain and can detect interrupts >> even most of the SoC is powered off. The wakeup interrupt controller > >even when? > >> wakes up the GIC and replays the interrupt at the GIC. >> >> Setup the TLMM irqchip in hierarchy with the wakeup interrupt controller >> and ensure the wakeup GPIOs are handled correctly. >> >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > >Is it Co-developed-by for Maulik? > >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> > >Some minor comments. Shouldn't be hard to fix and resend quickly I hope. > Thanks for the review Stephen. I will fix these and resend. >> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c >> index 763da0b..c245686 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-msm.c >> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c >> @@ -44,6 +46,7 @@ >> * @enabled_irqs: Bitmap of currently enabled irqs. >> * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge >> * detection. >> + * @skip_wake_irqs: Skip IRQs that are handled by wakeup interrupt contrroller > >s/contrroller/controller/ > >> * @soc; Reference to soc_data of platform specific data. >> * @regs: Base addresses for the TLMM tiles. >> */ >> @@ -778,10 +794,37 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear) >> >> static void msm_gpio_irq_enable(struct irq_data *d) >> { >> + /* >> + * Clear the interrupt that may be pending before we enable >> + * the line. >> + * This is especially a problem with the GPIOs routed to the >> + * PDC. These GPIOs are direct-connect interrupts to the GIC. >> + * Disabling the interrupt line at the PDC does not prevent >> + * the interrupt from being latched at the GIC. The state at >> + * GIC needs to be cleared before enabling. >> + */ >> + if (d->parent_data) { >> + irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0); >> + irq_chip_enable_parent(d); >> + } >> >> msm_gpio_irq_clear_unmask(d, true); >> } >> >> +static void msm_gpio_irq_disable(struct irq_data *d) >> +{ >> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); >> + struct msm_pinctrl *pctrl = gpiochip_get_data(gc); >> + >> + if (d->parent_data) >> + irq_chip_disable_parent(d); >> + >> + if (test_bit(d->hwirq, pctrl->skip_wake_irqs)) >> + return; >> + >> + msm_gpio_irq_mask(d); > >Why not > > if (!test_bit(...) > msm_gpio_irq_mask(d); > >> +} >> + >> static void msm_gpio_irq_unmask(struct irq_data *d) >> { >> msm_gpio_irq_clear_unmask(d, false); >> @@ -912,6 +964,15 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) >> struct msm_pinctrl *pctrl = gpiochip_get_data(gc); >> unsigned long flags; >> >> + if (d->parent_data) >> + irq_chip_set_wake_parent(d, on); >> + >> + /* >> + * While they may not wake up when the TLMM is powered off, >> + * some GPIOs would like to wakeup the system from suspend >> + * when TLMM is powered on. To allow that, enable the GPIO >> + * summary line to be wakeup capable at GIC. >> + */ > >Can this comment go above the irq_set_irq_wake() line below instead of >this spinlock? > Sure. >> raw_spin_lock_irqsave(&pctrl->lock, flags); >> >> irq_set_irq_wake(pctrl->irq, on); >> @@ -990,6 +1051,30 @@ static void msm_gpio_irq_handler(struct irq_desc *desc) >> chained_irq_exit(chip, desc); >> } >> >> +static int msm_gpio_wakeirq(struct gpio_chip *gc, >> + unsigned int child, >> + unsigned int child_type, >> + unsigned int *parent, >> + unsigned int *parent_type) >> +{ >> + struct msm_pinctrl *pctrl = gpiochip_get_data(gc); >> + const struct msm_gpio_wakeirq_map *map; >> + int i; >> + >> + *parent = GPIO_NO_WAKE_IRQ; >> + *parent_type = IRQ_TYPE_EDGE_RISING; >> + >> + for (i = 0; i < pctrl->soc->nwakeirq_map; i++) { >> + map = &pctrl->soc->wakeirq_map[i]; >> + if (map->gpio == child) { >> + *parent = map->wakeirq; >> + break; >> + } >> + } >> + >> + return 0; > >Shouldn't we return -EINVAL if we can't translate the gpio irq to the >parent domain? I would expect to see return -EINVAL here and the above >if condition to return 0 instead of break. > Good catch. Sure. >> +} >> + >> static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl) >> { >> if (pctrl->soc->reserved_gpios) >> @@ -1004,6 +1089,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) >> struct gpio_irq_chip *girq; >> int ret; >> unsigned ngpio = pctrl->soc->ngpios; >> + struct device_node *np; >> >> if (WARN_ON(ngpio > MAX_NR_GPIO)) >> return -EINVAL; >> @@ -1020,17 +1106,44 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) >> >> pctrl->irq_chip.name = "msmgpio"; >> pctrl->irq_chip.irq_enable = msm_gpio_irq_enable; >> + pctrl->irq_chip.irq_disable = msm_gpio_irq_disable; >> 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; >> + pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent; >> pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type; >> pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake; >> pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres; >> pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres; >> >> + np = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0); >> + if (np) { >> + int i; >> + bool skip; >> + unsigned int gpio; > >Can these be placed at the top of this function instead of buried >halfway down here? > >> + >> + chip->irq.parent_domain = irq_find_matching_host(np, >> + DOMAIN_BUS_WAKEUP); >> + of_node_put(np); >> + if (!chip->irq.parent_domain) >> + return -EPROBE_DEFER; >> + chip->irq.child_to_parent_hwirq = msm_gpio_wakeirq; >> + >> + /* >> + * Let's skip handling the GPIOs, if the parent irqchip >> + * handling the direct connect IRQ of the GPIO. > >is handling? > >> + */ >> + skip = irq_domain_qcom_handle_wakeup(chip->irq.parent_domain); >> + for (i = 0; skip && i < pctrl->soc->nwakeirq_map; i++) { >> + gpio = pctrl->soc->wakeirq_map[i].gpio; >> + set_bit(gpio, pctrl->skip_wake_irqs); >> + } >> + } >> + >> girq = &chip->irq; >> girq->chip = &pctrl->irq_chip; >> girq->parent_handler = msm_gpio_irq_handler; >> + girq->fwnode = pctrl->dev->fwnode; >> girq->num_parents = 1; >> girq->parents = devm_kcalloc(pctrl->dev, 1, sizeof(*girq->parents), >> GFP_KERNEL); >> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h >> index 48569cda8..1547020 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-msm.h >> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h >> @@ -5,6 +5,8 @@ >> #ifndef __PINCTRL_MSM_H__ >> #define __PINCTRL_MSM_H__ >> >> +#include <linux/gpio/driver.h> > >What is this include for? > Must be from an older version. Will remove. >> + >> struct pinctrl_pin_desc; >> >> /** >> @@ -101,6 +113,8 @@ struct msm_pingroup { >> * @ngroups: The numbmer of entries in @groups. >> * @ngpio: The number of pingroups the driver should expose as GPIOs. >> * @pull_no_keeper: The SoC does not support keeper bias. >> + * @wakeirq_map: The map of wakeup capable GPIOs and the pin at PDC/MPM >> + * @nwakeirq_map: The number of entries in @hierarchy_map > >Is it 'number of entries in @wakeirq_map"? > Yes. Thanks. >> */ >> struct msm_pinctrl_soc_data { >> const struct pinctrl_pin_desc *pins; >> @@ -114,6 +128,8 @@ struct msm_pinctrl_soc_data { >> const char *const *tiles; >> unsigned int ntiles; >> const int *reserved_gpios; >> + const struct msm_gpio_wakeirq_map *wakeirq_map; >> + unsigned int nwakeirq_map; >> }; >> >> extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops; >> diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h >> index 637c0bf..e01391c 100644 >> --- a/include/linux/soc/qcom/irq.h >> +++ b/include/linux/soc/qcom/irq.h >> @@ -18,4 +18,17 @@ >> #define IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP (IRQ_DOMAIN_FLAG_NONCORE << 0) >> #define IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP (IRQ_DOMAIN_FLAG_NONCORE << 1) >> >> +/** >> + * irq_domain_qcom_handle_wakeup: Return if the domain handles interrupt >> + * configuration >> + * @d: irq domain >> + * >> + * This QCOM specific irq domain call returns if the interrupt controller >> + * requires the interrupt be masked at the child interrupt controller. >> + */ >> +static inline bool irq_domain_qcom_handle_wakeup(struct irq_domain *d) > >could be const irq_domain here. > Ok. >> +{ >> + return (d->flags & IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP); >> +} >> + Thanks, Lina
On Fri, Nov 15 2019 at 13:55 -0700, Lina Iyer wrote: >>Quoting Lina Iyer (2019-11-14 10:35:17) >>>+static int msm_gpio_wakeirq(struct gpio_chip *gc, >>>+ unsigned int child, >>>+ unsigned int child_type, >>>+ unsigned int *parent, >>>+ unsigned int *parent_type) >>>+{ >>>+ struct msm_pinctrl *pctrl = gpiochip_get_data(gc); >>>+ const struct msm_gpio_wakeirq_map *map; >>>+ int i; >>>+ >>>+ *parent = GPIO_NO_WAKE_IRQ; >>>+ *parent_type = IRQ_TYPE_EDGE_RISING; >>>+ >>>+ for (i = 0; i < pctrl->soc->nwakeirq_map; i++) { >>>+ map = &pctrl->soc->wakeirq_map[i]; >>>+ if (map->gpio == child) { >>>+ *parent = map->wakeirq; >>>+ break; >>>+ } >>>+ } >>>+ >>>+ return 0; >> >>Shouldn't we return -EINVAL if we can't translate the gpio irq to the >>parent domain? I would expect to see return -EINVAL here and the above >>if condition to return 0 instead of break. >> >Good catch. Sure. >>>+} >>>+ Looking into this, we have been setting GPIO in hierarchy with PDC and the return 0 is appropriate as it would set the GPIO_NO_WAKE_IRQ as the parent and setup the IRQ correctly. We fail to setup GPIOs otherwise.
Quoting Lina Iyer (2019-11-15 13:57:37) > On Fri, Nov 15 2019 at 13:55 -0700, Lina Iyer wrote: > >>Quoting Lina Iyer (2019-11-14 10:35:17) > > >>>+static int msm_gpio_wakeirq(struct gpio_chip *gc, > >>>+ unsigned int child, > >>>+ unsigned int child_type, > >>>+ unsigned int *parent, > >>>+ unsigned int *parent_type) > >>>+{ > >>>+ struct msm_pinctrl *pctrl = gpiochip_get_data(gc); > >>>+ const struct msm_gpio_wakeirq_map *map; > >>>+ int i; > >>>+ > >>>+ *parent = GPIO_NO_WAKE_IRQ; > >>>+ *parent_type = IRQ_TYPE_EDGE_RISING; > >>>+ > >>>+ for (i = 0; i < pctrl->soc->nwakeirq_map; i++) { > >>>+ map = &pctrl->soc->wakeirq_map[i]; > >>>+ if (map->gpio == child) { > >>>+ *parent = map->wakeirq; > >>>+ break; > >>>+ } > >>>+ } > >>>+ > >>>+ return 0; > >> > >>Shouldn't we return -EINVAL if we can't translate the gpio irq to the > >>parent domain? I would expect to see return -EINVAL here and the above > >>if condition to return 0 instead of break. > >> > >Good catch. Sure. > >>>+} > >>>+ > Looking into this, we have been setting GPIO in hierarchy with PDC and > the return 0 is appropriate as it would set the GPIO_NO_WAKE_IRQ as the > parent and setup the IRQ correctly. We fail to setup GPIOs otherwise. Ah ok so by default we will set the parent irq to ~0 and that means bypass pdc and go directly to GIC? Where do we fail to setup a GPIO otherwise? It sounds like we can always translate to either something in the map or to ~0.
On Fri, Nov 15 2019 at 15:09 -0700, Stephen Boyd wrote: >Quoting Lina Iyer (2019-11-15 13:57:37) >> On Fri, Nov 15 2019 at 13:55 -0700, Lina Iyer wrote: >> >>Quoting Lina Iyer (2019-11-14 10:35:17) >> >> >>>+static int msm_gpio_wakeirq(struct gpio_chip *gc, >> >>>+ unsigned int child, >> >>>+ unsigned int child_type, >> >>>+ unsigned int *parent, >> >>>+ unsigned int *parent_type) >> >>>+{ >> >>>+ struct msm_pinctrl *pctrl = gpiochip_get_data(gc); >> >>>+ const struct msm_gpio_wakeirq_map *map; >> >>>+ int i; >> >>>+ >> >>>+ *parent = GPIO_NO_WAKE_IRQ; >> >>>+ *parent_type = IRQ_TYPE_EDGE_RISING; >> >>>+ >> >>>+ for (i = 0; i < pctrl->soc->nwakeirq_map; i++) { >> >>>+ map = &pctrl->soc->wakeirq_map[i]; >> >>>+ if (map->gpio == child) { >> >>>+ *parent = map->wakeirq; >> >>>+ break; >> >>>+ } >> >>>+ } >> >>>+ >> >>>+ return 0; >> >> >> >>Shouldn't we return -EINVAL if we can't translate the gpio irq to the >> >>parent domain? I would expect to see return -EINVAL here and the above >> >>if condition to return 0 instead of break. >> >> >> >Good catch. Sure. >> >>>+} >> >>>+ >> Looking into this, we have been setting GPIO in hierarchy with PDC and >> the return 0 is appropriate as it would set the GPIO_NO_WAKE_IRQ as the >> parent and setup the IRQ correctly. We fail to setup GPIOs otherwise. > >Ah ok so by default we will set the parent irq to ~0 and that means >bypass pdc and go directly to GIC? > >Where do we fail to setup a GPIO otherwise? It sounds like we can always >translate to either something in the map or to ~0. > We do not, may be other architectures can use this check better. --lina
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index 763da0b..c245686 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -23,6 +23,8 @@ #include <linux/pm.h> #include <linux/log2.h> +#include <linux/soc/qcom/irq.h> + #include "../core.h" #include "../pinconf.h" #include "pinctrl-msm.h" @@ -44,6 +46,7 @@ * @enabled_irqs: Bitmap of currently enabled irqs. * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge * detection. + * @skip_wake_irqs: Skip IRQs that are handled by wakeup interrupt contrroller * @soc; Reference to soc_data of platform specific data. * @regs: Base addresses for the TLMM tiles. */ @@ -61,6 +64,7 @@ struct msm_pinctrl { DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO); DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO); + DECLARE_BITMAP(skip_wake_irqs, MAX_NR_GPIO); const struct msm_pinctrl_soc_data *soc; void __iomem *regs[MAX_NR_TILES]; @@ -707,6 +711,12 @@ static void msm_gpio_irq_mask(struct irq_data *d) unsigned long flags; u32 val; + if (d->parent_data) + irq_chip_mask_parent(d); + + if (test_bit(d->hwirq, pctrl->skip_wake_irqs)) + return; + g = &pctrl->soc->groups[d->hwirq]; raw_spin_lock_irqsave(&pctrl->lock, flags); @@ -751,6 +761,12 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear) unsigned long flags; u32 val; + if (d->parent_data) + irq_chip_unmask_parent(d); + + if (test_bit(d->hwirq, pctrl->skip_wake_irqs)) + return; + g = &pctrl->soc->groups[d->hwirq]; raw_spin_lock_irqsave(&pctrl->lock, flags); @@ -778,10 +794,37 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear) static void msm_gpio_irq_enable(struct irq_data *d) { + /* + * Clear the interrupt that may be pending before we enable + * the line. + * This is especially a problem with the GPIOs routed to the + * PDC. These GPIOs are direct-connect interrupts to the GIC. + * Disabling the interrupt line at the PDC does not prevent + * the interrupt from being latched at the GIC. The state at + * GIC needs to be cleared before enabling. + */ + if (d->parent_data) { + irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0); + irq_chip_enable_parent(d); + } msm_gpio_irq_clear_unmask(d, true); } +static void msm_gpio_irq_disable(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct msm_pinctrl *pctrl = gpiochip_get_data(gc); + + if (d->parent_data) + irq_chip_disable_parent(d); + + if (test_bit(d->hwirq, pctrl->skip_wake_irqs)) + return; + + msm_gpio_irq_mask(d); +} + static void msm_gpio_irq_unmask(struct irq_data *d) { msm_gpio_irq_clear_unmask(d, false); @@ -795,6 +838,9 @@ static void msm_gpio_irq_ack(struct irq_data *d) unsigned long flags; u32 val; + if (test_bit(d->hwirq, pctrl->skip_wake_irqs)) + return; + g = &pctrl->soc->groups[d->hwirq]; raw_spin_lock_irqsave(&pctrl->lock, flags); @@ -820,6 +866,12 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) unsigned long flags; u32 val; + if (d->parent_data) + irq_chip_set_type_parent(d, type); + + if (test_bit(d->hwirq, pctrl->skip_wake_irqs)) + return 0; + g = &pctrl->soc->groups[d->hwirq]; raw_spin_lock_irqsave(&pctrl->lock, flags); @@ -912,6 +964,15 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) struct msm_pinctrl *pctrl = gpiochip_get_data(gc); unsigned long flags; + if (d->parent_data) + irq_chip_set_wake_parent(d, on); + + /* + * While they may not wake up when the TLMM is powered off, + * some GPIOs would like to wakeup the system from suspend + * when TLMM is powered on. To allow that, enable the GPIO + * summary line to be wakeup capable at GIC. + */ raw_spin_lock_irqsave(&pctrl->lock, flags); irq_set_irq_wake(pctrl->irq, on); @@ -990,6 +1051,30 @@ static void msm_gpio_irq_handler(struct irq_desc *desc) chained_irq_exit(chip, desc); } +static int msm_gpio_wakeirq(struct gpio_chip *gc, + unsigned int child, + unsigned int child_type, + unsigned int *parent, + unsigned int *parent_type) +{ + struct msm_pinctrl *pctrl = gpiochip_get_data(gc); + const struct msm_gpio_wakeirq_map *map; + int i; + + *parent = GPIO_NO_WAKE_IRQ; + *parent_type = IRQ_TYPE_EDGE_RISING; + + for (i = 0; i < pctrl->soc->nwakeirq_map; i++) { + map = &pctrl->soc->wakeirq_map[i]; + if (map->gpio == child) { + *parent = map->wakeirq; + break; + } + } + + return 0; +} + static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl) { if (pctrl->soc->reserved_gpios) @@ -1004,6 +1089,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) struct gpio_irq_chip *girq; int ret; unsigned ngpio = pctrl->soc->ngpios; + struct device_node *np; if (WARN_ON(ngpio > MAX_NR_GPIO)) return -EINVAL; @@ -1020,17 +1106,44 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) pctrl->irq_chip.name = "msmgpio"; pctrl->irq_chip.irq_enable = msm_gpio_irq_enable; + pctrl->irq_chip.irq_disable = msm_gpio_irq_disable; 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; + pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent; pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type; pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake; pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres; pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres; + np = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0); + if (np) { + int i; + bool skip; + unsigned int gpio; + + chip->irq.parent_domain = irq_find_matching_host(np, + DOMAIN_BUS_WAKEUP); + of_node_put(np); + if (!chip->irq.parent_domain) + return -EPROBE_DEFER; + chip->irq.child_to_parent_hwirq = msm_gpio_wakeirq; + + /* + * Let's skip handling the GPIOs, if the parent irqchip + * handling the direct connect IRQ of the GPIO. + */ + skip = irq_domain_qcom_handle_wakeup(chip->irq.parent_domain); + for (i = 0; skip && i < pctrl->soc->nwakeirq_map; i++) { + gpio = pctrl->soc->wakeirq_map[i].gpio; + set_bit(gpio, pctrl->skip_wake_irqs); + } + } + girq = &chip->irq; girq->chip = &pctrl->irq_chip; girq->parent_handler = msm_gpio_irq_handler; + girq->fwnode = pctrl->dev->fwnode; girq->num_parents = 1; girq->parents = devm_kcalloc(pctrl->dev, 1, sizeof(*girq->parents), GFP_KERNEL); diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h index 48569cda8..1547020 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.h +++ b/drivers/pinctrl/qcom/pinctrl-msm.h @@ -5,6 +5,8 @@ #ifndef __PINCTRL_MSM_H__ #define __PINCTRL_MSM_H__ +#include <linux/gpio/driver.h> + struct pinctrl_pin_desc; /** @@ -92,6 +94,16 @@ struct msm_pingroup { }; /** + * struct msm_gpio_wakeirq_map - Map of GPIOs and their wakeup pins + * @gpio: The GPIOs that are wakeup capable + * @wakeirq: The interrupt at the always-on interrupt controller + */ +struct msm_gpio_wakeirq_map { + unsigned int gpio; + unsigned int wakeirq; +}; + +/** * struct msm_pinctrl_soc_data - Qualcomm pin controller driver configuration * @pins: An array describing all pins the pin controller affects. * @npins: The number of entries in @pins. @@ -101,6 +113,8 @@ struct msm_pingroup { * @ngroups: The numbmer of entries in @groups. * @ngpio: The number of pingroups the driver should expose as GPIOs. * @pull_no_keeper: The SoC does not support keeper bias. + * @wakeirq_map: The map of wakeup capable GPIOs and the pin at PDC/MPM + * @nwakeirq_map: The number of entries in @hierarchy_map */ struct msm_pinctrl_soc_data { const struct pinctrl_pin_desc *pins; @@ -114,6 +128,8 @@ struct msm_pinctrl_soc_data { const char *const *tiles; unsigned int ntiles; const int *reserved_gpios; + const struct msm_gpio_wakeirq_map *wakeirq_map; + unsigned int nwakeirq_map; }; extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops; diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h index 637c0bf..e01391c 100644 --- a/include/linux/soc/qcom/irq.h +++ b/include/linux/soc/qcom/irq.h @@ -18,4 +18,17 @@ #define IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP (IRQ_DOMAIN_FLAG_NONCORE << 0) #define IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP (IRQ_DOMAIN_FLAG_NONCORE << 1) +/** + * irq_domain_qcom_handle_wakeup: Return if the domain handles interrupt + * configuration + * @d: irq domain + * + * This QCOM specific irq domain call returns if the interrupt controller + * requires the interrupt be masked at the child interrupt controller. + */ +static inline bool irq_domain_qcom_handle_wakeup(struct irq_domain *d) +{ + return (d->flags & IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP); +} + #endif