[08/12] drivers: pinctrl: msm: setup GPIO chip in hierarchy
diff mbox series

Message ID 1573756521-27373-9-git-send-email-ilina@codeaurora.org
State New
Headers show
Series
  • Support wakeup capable GPIOs
Related show

Commit Message

Lina Iyer Nov. 14, 2019, 6:35 p.m. UTC
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>
----
Changes in v1:
	- Address minor review comments
	- Remove redundant call to set irq handler
	- Move irq_domain_qcom_handle_wakeup() to this patch
Changes in RFC v2:
	- Rebase on top of GPIO hierarchy support in linux-next
	- Set the chained irq handler for summary line
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 113 +++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/qcom/pinctrl-msm.h |  16 ++++++
 include/linux/soc/qcom/irq.h       |  13 +++++
 3 files changed, 142 insertions(+)

Comments

Linus Walleij Nov. 14, 2019, 6:43 p.m. UTC | #1
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
Lina Iyer Nov. 14, 2019, 6:57 p.m. UTC | #2
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
Stephen Boyd Nov. 15, 2019, 7:33 p.m. UTC | #3
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);
> +}
> +
Lina Iyer Nov. 15, 2019, 8:55 p.m. UTC | #4
>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
Lina Iyer Nov. 15, 2019, 9:57 p.m. UTC | #5
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.
Stephen Boyd Nov. 15, 2019, 10:09 p.m. UTC | #6
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.
Lina Iyer Nov. 15, 2019, 10:13 p.m. UTC | #7
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

Patch
diff mbox series

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