Re: [PATCH 08/12] drivers: pinctrl: msm: setup GPIO chip in hierarchy

[Lina Iyer]

> 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@xxxxxxxxxxxxxx>
>
> Is it Co-developed-by for Maulik?
>
> > Signed-off-by: Lina Iyer <ilina@xxxxxxxxxxxxxx>
>
> 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