diff mbox series

[v2,4/4] irqchip: qcom-pdc: Introduce irq_set_wake call

Message ID 1590253873-11556-5-git-send-email-mkshah@codeaurora.org
State New
Headers show
Series irqchip: qcom: pdc: Introduce irq_set_wake call | expand

Commit Message

Maulik Shah May 23, 2020, 5:11 p.m. UTC
Remove irq_disable callback to allow lazy disable for pdc interrupts.

Add irq_set_wake callback that unmask interrupt in HW when drivers
mark interrupt for wakeup. Interrupt will be cleared in HW during
lazy disable if its not marked for wakeup.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/irqchip/qcom-pdc.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

Comments

Stephen Boyd May 27, 2020, 10:15 a.m. UTC | #1
Quoting Maulik Shah (2020-05-23 10:11:13)
> Remove irq_disable callback to allow lazy disable for pdc interrupts.
> 
> Add irq_set_wake callback that unmask interrupt in HW when drivers
> mark interrupt for wakeup. Interrupt will be cleared in HW during
> lazy disable if its not marked for wakeup.
> 
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
>  drivers/irqchip/qcom-pdc.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index 6ae9e1f..f7c0662 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -36,6 +36,7 @@ struct pdc_pin_region {
>         u32 cnt;
>  };
>  
> +DECLARE_BITMAP(pdc_wake_irqs, PDC_MAX_IRQS);

static?

>  static DEFINE_RAW_SPINLOCK(pdc_lock);
>  static void __iomem *pdc_base;
>  static struct pdc_pin_region *pdc_region;
> @@ -87,22 +88,20 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
>         raw_spin_unlock(&pdc_lock);
>  }
>  
> -static void qcom_pdc_gic_disable(struct irq_data *d)
> +static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on)
>  {
>         if (d->hwirq == GPIO_NO_WAKE_IRQ)
> -               return;
> -
> -       pdc_enable_intr(d, false);
> -       irq_chip_disable_parent(d);
> -}
> +               return 0;

Shouldn't this fail if we can't set for wake?

>  
> -static void qcom_pdc_gic_enable(struct irq_data *d)
> -{
> -       if (d->hwirq == GPIO_NO_WAKE_IRQ)
> -               return;
> +       if (on) {
> +               pdc_enable_intr(d, true);
> +               irq_chip_enable_parent(d);
> +               set_bit(d->hwirq, pdc_wake_irqs);
> +       } else {
> +               clear_bit(d->hwirq, pdc_wake_irqs);
> +       }
>  
> -       pdc_enable_intr(d, true);
> -       irq_chip_enable_parent(d);
> +       return irq_chip_set_wake_parent(d, on);
>  }
>  
>  static void qcom_pdc_gic_mask(struct irq_data *d)

The diff is really hard to read too. Maybe set_wake can be added first
and then the enable/disable functions removed?

> @@ -110,6 +109,9 @@ static void qcom_pdc_gic_mask(struct irq_data *d)
>         if (d->hwirq == GPIO_NO_WAKE_IRQ)
>                 return;
>  
> +       if (!test_bit(d->hwirq, pdc_wake_irqs))
> +               pdc_enable_intr(d, false);
> +
>         irq_chip_mask_parent(d);
>  }
>  
> @@ -118,6 +120,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d)
>         if (d->hwirq == GPIO_NO_WAKE_IRQ)
>                 return;
>  
> +       pdc_enable_intr(d, true);
>         irq_chip_unmask_parent(d);
>  }
>  

I find these two hunks deeply confusing. I'm not sure what the
maintainers think though. I hope it would be simpler to always enable
the hwirqs in the pdc when an irq is requested and only disable it in
the pdc when the system goes to suspend and the pdc pin isn't for an irq
that's marked for wakeup. Does that break somehow?

My understanding of the hardware is that the GPIO controller has lines
directly connected to various SPI lines on the GIC and PDC has a way to
monitor those direct connections and wakeup the CPUs when they trigger
the detection logic in the PDC. The enable/disable bit in PDC gates that
logic for each wire between the GPIO controller and the GIC.

So isn't it simpler to leave the PDC monitoring pins that we care about
all the time and only stop monitoring when we enter and leave suspend?
And shouldn't the driver set something sane in qcom_pdc_init() to
disable all the pdc pins so that we don't rely on boot state to
configure pins for wakeup?

> @@ -197,15 +200,13 @@ static struct irq_chip qcom_pdc_gic_chip = {
>         .irq_eoi                = irq_chip_eoi_parent,
>         .irq_mask               = qcom_pdc_gic_mask,
>         .irq_unmask             = qcom_pdc_gic_unmask,
> -       .irq_disable            = qcom_pdc_gic_disable,
> -       .irq_enable             = qcom_pdc_gic_enable,
>         .irq_get_irqchip_state  = qcom_pdc_gic_get_irqchip_state,
>         .irq_set_irqchip_state  = qcom_pdc_gic_set_irqchip_state,
>         .irq_retrigger          = irq_chip_retrigger_hierarchy,
>         .irq_set_type           = qcom_pdc_gic_set_type,
> +       .irq_set_wake           = qcom_pdc_gic_set_wake,
>         .flags                  = IRQCHIP_MASK_ON_SUSPEND |
> -                                 IRQCHIP_SET_TYPE_MASKED |
> -                                 IRQCHIP_SKIP_SET_WAKE,
> +                                 IRQCHIP_SET_TYPE_MASKED,
>         .irq_set_vcpu_affinity  = irq_chip_set_vcpu_affinity_parent,
>         .irq_set_affinity       = irq_chip_set_affinity_parent,
Maulik Shah May 29, 2020, 9:20 a.m. UTC | #2
Hi,

On 5/27/2020 3:45 PM, Stephen Boyd wrote:
> Quoting Maulik Shah (2020-05-23 10:11:13)
>> Remove irq_disable callback to allow lazy disable for pdc interrupts.
>>
>> Add irq_set_wake callback that unmask interrupt in HW when drivers
>> mark interrupt for wakeup. Interrupt will be cleared in HW during
>> lazy disable if its not marked for wakeup.
>>
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>> ---
>>   drivers/irqchip/qcom-pdc.c | 33 +++++++++++++++++----------------
>>   1 file changed, 17 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
>> index 6ae9e1f..f7c0662 100644
>> --- a/drivers/irqchip/qcom-pdc.c
>> +++ b/drivers/irqchip/qcom-pdc.c
>> @@ -36,6 +36,7 @@ struct pdc_pin_region {
>>          u32 cnt;
>>   };
>>   
>> +DECLARE_BITMAP(pdc_wake_irqs, PDC_MAX_IRQS);
> static?
Thanks i will declare as static in v3.
>
>>   static DEFINE_RAW_SPINLOCK(pdc_lock);
>>   static void __iomem *pdc_base;
>>   static struct pdc_pin_region *pdc_region;
>> @@ -87,22 +88,20 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
>>          raw_spin_unlock(&pdc_lock);
>>   }
>>   
>> -static void qcom_pdc_gic_disable(struct irq_data *d)
>> +static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on)
>>   {
>>          if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> -               return;
>> -
>> -       pdc_enable_intr(d, false);
>> -       irq_chip_disable_parent(d);
>> -}
>> +               return 0;
> Shouldn't this fail if we can't set for wake?

we return success/failure from parent chip with below call at end of 
set_wake.

return irq_chip_set_wake_parent(d, on);

>
>>   
>> -static void qcom_pdc_gic_enable(struct irq_data *d)
>> -{
>> -       if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> -               return;
>> +       if (on) {
>> +               pdc_enable_intr(d, true);
>> +               irq_chip_enable_parent(d);
>> +               set_bit(d->hwirq, pdc_wake_irqs);
>> +       } else {
>> +               clear_bit(d->hwirq, pdc_wake_irqs);
>> +       }
>>   
>> -       pdc_enable_intr(d, true);
>> -       irq_chip_enable_parent(d);
>> +       return irq_chip_set_wake_parent(d, on);
>>   }
>>   
>>   static void qcom_pdc_gic_mask(struct irq_data *d)
> The diff is really hard to read too. Maybe set_wake can be added first
> and then the enable/disable functions removed?
i think should be ok in same patch, if you insist i can split this 
change in to two.
>
>> @@ -110,6 +109,9 @@ static void qcom_pdc_gic_mask(struct irq_data *d)
>>          if (d->hwirq == GPIO_NO_WAKE_IRQ)
>>                  return;
>>   
>> +       if (!test_bit(d->hwirq, pdc_wake_irqs))
>> +               pdc_enable_intr(d, false);
>> +
>>          irq_chip_mask_parent(d);
>>   }
>>   
>> @@ -118,6 +120,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d)
>>          if (d->hwirq == GPIO_NO_WAKE_IRQ)
>>                  return;
>>   
>> +       pdc_enable_intr(d, true);
>>          irq_chip_unmask_parent(d);
>>   }
>>   
> I find these two hunks deeply confusing. I'm not sure what the
> maintainers think though. I hope it would be simpler to always enable
> the hwirqs in the pdc when an irq is requested and only disable it in
> the pdc when the system goes to suspend and the pdc pin isn't for an irq
> that's marked for wakeup. Does that break somehow?
PDC monitors interrupts during CPUidle as well, in cases where deepest 
low power mode happened from cpuidle where GIC is not active.
If we keep PDC IRQ always enabled/unmasked during idle and then 
disable/mask when entering to suspend, it will break cpuidle.

>
> My understanding of the hardware is that the GPIO controller has lines
> directly connected to various SPI lines on the GIC and PDC has a way to
> monitor those direct connections and wakeup the CPUs when they trigger
> the detection logic in the PDC. The enable/disable bit in PDC gates that
> logic for each wire between the GPIO controller and the GIC.
>
> So isn't it simpler to leave the PDC monitoring pins that we care about
> all the time and only stop monitoring when we enter and leave suspend?

it can affect idle path as explained above.

> And shouldn't the driver set something sane in qcom_pdc_init() to
> disable all the pdc pins so that we don't rely on boot state to
> configure pins for wakeup?

We don't rely on boot state, by default all interrupt will be disabled.

This is same to GIC driver having GICD_ISENABLER register, where all 
bits (one bit per interrupt) set to 0 (masked irqs) during boot up.

Similarly PDC also have all bits set to 0 in PDC's IRQ_ENABLE_BANK.

Thanks,
Maulik
>
>> @@ -197,15 +200,13 @@ static struct irq_chip qcom_pdc_gic_chip = {
>>          .irq_eoi                = irq_chip_eoi_parent,
>>          .irq_mask               = qcom_pdc_gic_mask,
>>          .irq_unmask             = qcom_pdc_gic_unmask,
>> -       .irq_disable            = qcom_pdc_gic_disable,
>> -       .irq_enable             = qcom_pdc_gic_enable,
>>          .irq_get_irqchip_state  = qcom_pdc_gic_get_irqchip_state,
>>          .irq_set_irqchip_state  = qcom_pdc_gic_set_irqchip_state,
>>          .irq_retrigger          = irq_chip_retrigger_hierarchy,
>>          .irq_set_type           = qcom_pdc_gic_set_type,
>> +       .irq_set_wake           = qcom_pdc_gic_set_wake,
>>          .flags                  = IRQCHIP_MASK_ON_SUSPEND |
>> -                                 IRQCHIP_SET_TYPE_MASKED |
>> -                                 IRQCHIP_SKIP_SET_WAKE,
>> +                                 IRQCHIP_SET_TYPE_MASKED,
>>          .irq_set_vcpu_affinity  = irq_chip_set_vcpu_affinity_parent,
>>          .irq_set_affinity       = irq_chip_set_affinity_parent,
Stephen Boyd May 30, 2020, 7:26 p.m. UTC | #3
Quoting Maulik Shah (2020-05-29 02:20:32)
> Hi,
> 
> On 5/27/2020 3:45 PM, Stephen Boyd wrote:
> > Quoting Maulik Shah (2020-05-23 10:11:13)
> >> @@ -87,22 +88,20 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
> >>          raw_spin_unlock(&pdc_lock);
> >>   }
> >>   
> >> -static void qcom_pdc_gic_disable(struct irq_data *d)
> >> +static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on)
> >>   {
> >>          if (d->hwirq == GPIO_NO_WAKE_IRQ)
> >> -               return;
> >> -
> >> -       pdc_enable_intr(d, false);
> >> -       irq_chip_disable_parent(d);
> >> -}
> >> +               return 0;
> > Shouldn't this fail if we can't set for wake?
> 
> we return success/failure from parent chip with below call at end of 
> set_wake.
> 
> return irq_chip_set_wake_parent(d, on);

It's not a question about the parent irqchip. I'm wondering why we would
return success for a gpio irq that can't be marked for wakeup when a
client driver tries to enable wake on it. My understanding is that all
gpios irqs call here and PDC can't monitor all of them so some are
GPIO_NO_WAKE_IRQ and thus trying to mark those for wakeup should fail.
Of course msm_gpio_irq_set_wake() should also fail if it can't mark the
gpio for wakeup, but that's another problem.

> >> @@ -118,6 +120,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d)
> >>          if (d->hwirq == GPIO_NO_WAKE_IRQ)
> >>                  return;
> >>   
> >> +       pdc_enable_intr(d, true);
> >>          irq_chip_unmask_parent(d);
> >>   }
> >>   
> > I find these two hunks deeply confusing. I'm not sure what the
> > maintainers think though. I hope it would be simpler to always enable
> > the hwirqs in the pdc when an irq is requested and only disable it in
> > the pdc when the system goes to suspend and the pdc pin isn't for an irq
> > that's marked for wakeup. Does that break somehow?
> PDC monitors interrupts during CPUidle as well, in cases where deepest 
> low power mode happened from cpuidle where GIC is not active.
> If we keep PDC IRQ always enabled/unmasked during idle and then 
> disable/mask when entering to suspend, it will break cpuidle.

How does it break cpuidle? The irqs that would be enabled/unmasked in
pdc would only be the irqs that the kernel has setup irq handlers for
(from request_irq() and friends).  We want those irqs to keep working
during cpuidle and wake the CPU from the deepest idle states.

> 
> >
> > My understanding of the hardware is that the GPIO controller has lines
> > directly connected to various SPI lines on the GIC and PDC has a way to
> > monitor those direct connections and wakeup the CPUs when they trigger
> > the detection logic in the PDC. The enable/disable bit in PDC gates that
> > logic for each wire between the GPIO controller and the GIC.
> >
> > So isn't it simpler to leave the PDC monitoring pins that we care about
> > all the time and only stop monitoring when we enter and leave suspend?
> 
> it can affect idle path as explained above.
> 
> > And shouldn't the driver set something sane in qcom_pdc_init() to
> > disable all the pdc pins so that we don't rely on boot state to
> > configure pins for wakeup?
> 
> We don't rely on boot state, by default all interrupt will be disabled.

Does 'default' mean the hardware register reset state? I'm worried that
we will kexec and then various pdc pins will be enabled because the
previous kernel had them enabled but then the new kernel doesn't care
about those pins and we'll never be able to suspend or go idle. I don't
know what happens in the GIC case but I think gic_dist_config() and
things set a sane state at kernel boot.

> 
> This is same to GIC driver having GICD_ISENABLER register, where all 
> bits (one bit per interrupt) set to 0 (masked irqs) during boot up.
> 
> Similarly PDC also have all bits set to 0 in PDC's IRQ_ENABLE_BANK.
> 

What code sets the IRQ_ENABLE_BANK to all zero when this driver probes?
diff mbox series

Patch

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 6ae9e1f..f7c0662 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -36,6 +36,7 @@  struct pdc_pin_region {
 	u32 cnt;
 };
 
+DECLARE_BITMAP(pdc_wake_irqs, PDC_MAX_IRQS);
 static DEFINE_RAW_SPINLOCK(pdc_lock);
 static void __iomem *pdc_base;
 static struct pdc_pin_region *pdc_region;
@@ -87,22 +88,20 @@  static void pdc_enable_intr(struct irq_data *d, bool on)
 	raw_spin_unlock(&pdc_lock);
 }
 
-static void qcom_pdc_gic_disable(struct irq_data *d)
+static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on)
 {
 	if (d->hwirq == GPIO_NO_WAKE_IRQ)
-		return;
-
-	pdc_enable_intr(d, false);
-	irq_chip_disable_parent(d);
-}
+		return 0;
 
-static void qcom_pdc_gic_enable(struct irq_data *d)
-{
-	if (d->hwirq == GPIO_NO_WAKE_IRQ)
-		return;
+	if (on) {
+		pdc_enable_intr(d, true);
+		irq_chip_enable_parent(d);
+		set_bit(d->hwirq, pdc_wake_irqs);
+	} else {
+		clear_bit(d->hwirq, pdc_wake_irqs);
+	}
 
-	pdc_enable_intr(d, true);
-	irq_chip_enable_parent(d);
+	return irq_chip_set_wake_parent(d, on);
 }
 
 static void qcom_pdc_gic_mask(struct irq_data *d)
@@ -110,6 +109,9 @@  static void qcom_pdc_gic_mask(struct irq_data *d)
 	if (d->hwirq == GPIO_NO_WAKE_IRQ)
 		return;
 
+	if (!test_bit(d->hwirq, pdc_wake_irqs))
+		pdc_enable_intr(d, false);
+
 	irq_chip_mask_parent(d);
 }
 
@@ -118,6 +120,7 @@  static void qcom_pdc_gic_unmask(struct irq_data *d)
 	if (d->hwirq == GPIO_NO_WAKE_IRQ)
 		return;
 
+	pdc_enable_intr(d, true);
 	irq_chip_unmask_parent(d);
 }
 
@@ -197,15 +200,13 @@  static struct irq_chip qcom_pdc_gic_chip = {
 	.irq_eoi		= irq_chip_eoi_parent,
 	.irq_mask		= qcom_pdc_gic_mask,
 	.irq_unmask		= qcom_pdc_gic_unmask,
-	.irq_disable		= qcom_pdc_gic_disable,
-	.irq_enable		= qcom_pdc_gic_enable,
 	.irq_get_irqchip_state	= qcom_pdc_gic_get_irqchip_state,
 	.irq_set_irqchip_state	= qcom_pdc_gic_set_irqchip_state,
 	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 	.irq_set_type		= qcom_pdc_gic_set_type,
+	.irq_set_wake		= qcom_pdc_gic_set_wake,
 	.flags			= IRQCHIP_MASK_ON_SUSPEND |
-				  IRQCHIP_SET_TYPE_MASKED |
-				  IRQCHIP_SKIP_SET_WAKE,
+				  IRQCHIP_SET_TYPE_MASKED,
 	.irq_set_vcpu_affinity	= irq_chip_set_vcpu_affinity_parent,
 	.irq_set_affinity	= irq_chip_set_affinity_parent,
 };